N3 parser: do not create formulas if the Turtle mode is activated by Tpt · Pull Request #1142 · RDFLib/rdflib (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation10 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Fixes #1141
Proposed Changes
- Do not attempt to build a default formula if the Turtle mode is activated
Hi @ashleysommer! Thank you for your feedback and sorry for the buggy change.
I have added a test to ensure there is no regression.
Indeed, having a null context was creating a blank node Id collision problem.
I have fixed it. Hopefully all tests should pass now.
Coverage increased (+0.06%) to 75.855% when pulling 328fab1 on Tpt:#1141 into 89cb369 on RDFLib:master.
| @@ -1745,7 +1746,7 @@ def newBlankNode(self, arg=None, uri=None, why=None): |
|---|
| return arg.newBlankNode(uri) |
| elif isinstance(arg, Graph) or arg is None: |
| self.counter += 1 |
| bn = BNode("n" + str(self.counter)) |
| bn = BNode("n%sb%s" % (self.uuid, self.counter)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to have to look into this a little deeper. I don't know if adding a UUID to the RDFSink is the best way to approach this.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UUID usage is what is currently done internally by the formula blank node generator
| # with formula |
|---|
| graph = Graph() |
| self.assertTrue(graph.store.formula_aware) |
| graph.load(BytesIO(file), format=format) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just FYI, in RDFLib 5.1.0 the graph.load() method will be deprecated as part of the Graph API cleanup process, and in 6.0.0 load() will be removed.
You will need to use graph.parse() instead. In this case where file is a string with the RDF data, use graph.parse(data=file).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Thank you! I'll fix it if the global PR design is accepted
@Tpt this PR will require further and more thorough review by the maintainers team. The reason being this section of code you're working on is literally 15 to 20 years old, hasn't changed in a very long time, and used by hundreds of thousands of people every day for critical processes. Every tiny little change here could cause unforeseen problems in places we haven't thought of.
Thank you for the review! Indeed, there is a significant risk of unforeseen problem. I found the blank node ID generation one but there might be others.
@Tpt
Sorry about the long delay in reviewing this.
I've done some extensive testing on this today, and I'm happy to say that its all good!
We can merge this, and this will open up the possibility of having more simplified store backends as Turtle Parser targets.
