-
Notifications
You must be signed in to change notification settings - Fork 135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[issue-561] fix rdf parser #563
Conversation
src/spdx/parser/rdf/rdf_parser.py
Outdated
for parent_node, _, element_node in graph.triples(triple): | ||
try: | ||
relationship = parse_implicit_relationship( | ||
element_node, graph, parent_node, creation_info.document_namespace, relationship_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this line PyCharm complains about a wrong type: "Expected type 'URIRef', got 'Node' instead". I am not sure how to solve this, the return type of graph.triples(..)
is Tuple[Node, Node, Node]
which is pretty general. parse_implicit_relationship
expects a URIRef
which should be fulfilled if the rdf file is valid. The only solution I could think of is to accept a Node
as input and check if element_node
is a URIRef
in parse_implicit_relationship
and log an error if this is not the case. Any other suggestions on how to fix this are much appreciated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your proposal is the right way to go. We can't expect a "random" triple from the parsed RDF graph to be a URIRef
, so we have to make sure that we get the correct type. I'm curious, though, why this doesn't pop up in other uses of graph.triples()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a wrapper method for the graph.triples(..)
and also for graph.value(..)
to ensure types at some places. I decided to write a Warning in the logger if the type doesn't match as the code would probably run either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting and fixing this! :)
I have a few small remarks.
src/spdx/parser/rdf/rdf_parser.py
Outdated
for parent_node, _, element_node in graph.triples(triple): | ||
try: | ||
relationship = parse_implicit_relationship( | ||
element_node, graph, parent_node, creation_info.document_namespace, relationship_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your proposal is the right way to go. We can't expect a "random" triple from the parsed RDF graph to be a URIRef
, so we have to make sure that we get the correct type. I'm curious, though, why this doesn't pop up in other uses of graph.triples()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments on the new triples functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now :)
…d "describesPackage" relationships Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
Signed-off-by: Meret Behrens <meret.behrens@tngtech.com>
f8115a6
to
af1dbfc
Compare
This PR adds some logic to parse implicit "hasFiles" and "describesPackage" relationships.
fixes #561