Skip to content

Conversation

@mbrush
Copy link
Collaborator

@mbrush mbrush commented Jan 19, 2023

Adding RetrievalSource object (and a reference to it form Edge) to support richer representations of retrieval provenance per #369.

Adding RetrievalSource object (and a reference to it form Edge) to support richer representations of retrieval provenance per NCATSTranslator#369.
Copy link
Collaborator

@edeutsch edeutsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

an Edge. Note that a given Edge should have one and only one 'primary'
source, and may have any number of 'aggregator' or 'supporting data'
sources.
enum:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these have biolink: prefixes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

@edeutsch edeutsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however, the build is failing because you have exceeded the dreaded 79 character limit.

@edeutsch
Copy link
Collaborator

super-fussy yamllint is still unhappy with this PR:

  875:26    error    trailing spaces  (trailing-spaces)

  877:63    error    trailing spaces  (trailing-spaces)

  878:71    error    trailing spaces  (trailing-spaces)

  879:68    error    trailing spaces  (trailing-spaces)

  881:26    error    trailing spaces  (trailing-spaces)

  1197:72   error    trailing spaces  (trailing-spaces)

  1199:61   error    trailing spaces  (trailing-spaces)

  1201:18   error    trailing spaces  (trailing-spaces)

  1204:74   error    trailing spaces  (trailing-spaces)

  1209:23   error    trailing spaces  (trailing-spaces)

  1212:71   error    trailing spaces  (trailing-spaces)

  1214:70   error    trailing spaces  (trailing-spaces)

  1222:17   error    trailing spaces  (trailing-spaces)

  1224:26   error    trailing spaces  (trailing-spaces)

  1225:68   error    trailing spaces  (trailing-spaces)

  1226:73   error    trailing spaces  (trailing-spaces)

  1227:76   error    trailing spaces  (trailing-spaces)

  1228:73   error    trailing spaces  (trailing-spaces)

  1229:75   error    trailing spaces  (trailing-spaces)

  1231:73   error    trailing spaces  (trailing-spaces)

  1232:69   error    trailing spaces  (trailing-spaces)

  1238:1    error    syntax error: found unexpected end of stream (syntax)

- primary knowledge source
- aggregator knowledge source
- supporting data source
previous_resource:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "upstream_resources" would be better?

  • upstream seems better than "previous"?
  • plural since an array?

remove trailing whitespaces, Edge.sources array minItems: 1, previous_resource -> upstream_resources, snake case enum values
@edeutsch
Copy link
Collaborator

Automatic checks are still failing. I think the problem is that on line 1124 there is an unclosed quote

@edeutsch
Copy link
Collaborator

The automatic build is still failing. The goal would be to ensure that the automatic validation passes.
yamllint is now happy but there are multiple OpenAPI schema violations. I think the easiest way to debug your yaml is to paste the entire yaml document in https://editor.swagger.io/ (after clearing the current content)
That interface is pretty good about showing you the schema problems.

@sierra-moxon
Copy link
Member

sierra-moxon commented Jan 28, 2023

I fixed the tests for this change in PR #393, this PR could be closed in favor of that one (it was easier to collaborate on a new branch in this repo, then try to fix this off of a fork of your fork off of Richard's fork @mbrush :) ).

@mbrush mbrush closed this Jan 29, 2023
@mbrush
Copy link
Collaborator Author

mbrush commented Jan 29, 2023

Replaced with PR #393 (in which @sierra-moxon fixed syntax errors that were causing test failures, and made resourceRole an object to reference so that it can leverage biolink enums)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants