Apply IdentifiedNode to Graph iterators #1697

ajnelson-nist · 2022-01-25T18:53:48Z

Proposed Changes

Use IdentifiedNode on Graph.triples() and related methods
Apply type signature revisions to compare.py and rdf-xml serializer.
(More to come after discussion on IdentifiedNode should be used in Graph.triples() and related iterators #1696 settles.)

Note the Graph signature updates should also be propagated to Graph subclasses before this is merged.

This patch is known to be incomplete with respect to other needed revisions to type signatures, both in Graph subclasses and in graph consumers elsewhere under `/rdflib`. However, running this change through a type checker raised some questions on `Node` specificity and circular type dependencies, and so is posted for discussion. References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

This resolves type signature issues raised in the prior patch, but might require a follow-on patch depending on `Path` discussion. References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

This patch makes no semantic changes, instead applying formatting from `black`. Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

ajnelson-nist · 2022-01-25T18:59:44Z

This is ready for look-overs. Let's keep discussion to #1696.

Thanks to Iwan for noting this variable's behavior. References: * RDFLib#1696 Reported-by: Iwan Aucamp <aucampia@gmail.com> Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

`mypy` was confused by some of the variable name re-use and/or re-definition. This patch also highlights that `Path`s can be yielded in the predicate position, which has further downstream consequences in `graph.py` and in the `Store` class. References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

aucampia · 2022-02-20T23:07:21Z

I will have a look later this week, but this is quite critical to do, so thanks. I will also see if I can make CI pipeline pass.

aucampia · 2022-02-22T20:30:56Z

rdflib/graph.py

+        self,
+        triple: Tuple[
+            Optional[IdentifiedNode], Union[None, Path, IdentifiedNode], Optional[Node]
+        ],
+    ) -> Iterable[Tuple[IdentifiedNode, Union[IdentifiedNode, Path], Node]]:


I think this is more correct than it was, but technically this should be:

Suggested change

self,

triple: Tuple[

Optional[IdentifiedNode], Union[None, Path, IdentifiedNode], Optional[Node]

],

) -> Iterable[Tuple[IdentifiedNode, Union[IdentifiedNode, Path], Node]]:

self,

triple: Tuple[

Optional[IdentifiedNode], Union[None, Path, URIRef], Optional[Node]

],

) -> Iterable[Tuple[IdentifiedNode, Union[URIRef, Path], Node]]:

Fine with it as is though, we can narrow it more later, this narrow already makes things better.

Same applies other in places also, won't label them all, just a note.

Type checking passes now. I also included some changes which reduces the footprint of this change so that it becomes almost entirely just change of type annotations. This may be a bit on the pendatic side, and I would be fine if you keep the changes as they were, but I would rather keep refactoring seperate from changes to add typing.

aucampia · 2022-02-22T21:49:43Z

Change looks very good, thanks, and it is quite essential, I made some minor fixes in ajnelson-nist#14 to get the type checking to pass, and I also roll back some changes which is not entirely essential for this PR's aim, but it's up to you what to do with those.

…de_changes Fix type checking errors

aucampia · 2022-02-23T00:17:32Z

For some reason mypy does not complain about unused ignores on my computer, will have to figure out why, but I removed the ignores that are not needed now and type validation seems to pass.

ajnelson-nist · 2022-02-23T00:20:25Z

Thanks! I just hit that too, and I'm dealing with a weirdness in rdflib/plugins/serializers/rdfxml.py, line 190:

rdflib/plugins/serializers/rdfxml.py:190: error: Incompatible types in assignment (expression has type "Set[IdentifiedNode]", variable has type "Set[Node]")
rdflib/plugins/serializers/rdfxml.py:190: error: Argument 1 to "union" of "set" has incompatible type "Iterable[Node]"; expected "Iterable[IdentifiedNode]"

Partially reverting your PR to split that up again is...almost helping. I'm close to pushing a patch once I figure out the spirit of the possibles variable.

aucampia · 2022-02-23T00:22:21Z

This validates on github actions now, I think maybe I have an older version of mypy than is being used on github actions

ajnelson-nist · 2022-02-23T00:26:21Z

Hm, seems I had an old-mypy issue too. I was on 0.920, upgraded to 0.931, and now that curious issue with possibles has gone away. (Though, squinting at that line, I think it'll be worth an independent PR.)

I'm going to take this PR out of Draft mode now. It seems ready for any last review and merge to me.

ajnelson-nist · 2022-02-23T00:29:12Z

...I spoke a little soon, I missed black wanted to reformat a bit. NOW I think it's ready.

aucampia · 2022-02-23T07:39:48Z

pre-commit.ci autofix

for more information, see https://pre-commit.ci

aucampia

This looks good to me, thanks again @ajnelson-nist

This only changes type annotations with one small exception, which is a change in variable names in well tested code path, this change in variable names is needed for type checking to pass without error/ignores as mypy does not like that the same variable has different types in the same scope:

rdflib/rdflib/graph.py

Lines 495 to 497 in 06feef0

    
           else: 
        
               for (_s, _p, _o), cg in self.__store.triples((s, p, o), context=self): 
        
                   yield _s, _p, _o

This PR also narrows some type annotations, but this narrowing brings things more in line with RDF 1.1 spec so I think it is good, if someone wants to use rdflib with non compliant types and type checking they will have to use # type: ignore or cast - but on the other hand, if we try and cater for non compliant RDF it also makes our lives harder.

The narrower types should however help users of this library detect errors in their code where they are potentially creating invalid RDF graphs.

I will add a page in docs regarding type annotations which covers some of the questions in #1696 and #1447

ajnelson-nist added 2 commits January 25, 2022 13:49

Apply IdentifiedNode type signature revisions to compare.py

a098176

This resolves type signature issues raised in the prior patch, but might require a follow-on patch depending on `Path` discussion. References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

ajnelson-nist marked this pull request as draft January 25, 2022 18:54

ajnelson-nist added 2 commits January 25, 2022 13:55

Apply IdentifiedNode type signature revisions to rdf-xml serializer

a813794

This resolves type signature issues raised in the prior patch, but might require a follow-on patch depending on `Path` discussion. References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

Reformat per black

246af8b

This patch makes no semantic changes, instead applying formatting from `black`. Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

ajnelson-nist mentioned this pull request Jan 25, 2022

IdentifiedNode should be used in Graph.triples() and related iterators #1696

Closed

ajnelson-nist added 3 commits January 26, 2022 06:53

Bypass circular import with TYPE_CHECKING

544731c

Thanks to Iwan for noting this variable's behavior. References: * RDFLib#1696 Reported-by: Iwan Aucamp <aucampia@gmail.com> Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

Add type signature for Path.eval()

5e78d82

References: * RDFLib#1696 Signed-off-by: Alex Nelson <alexander.nelson@nist.gov>

aucampia mentioned this pull request Feb 21, 2022

Remove singly-used alias obviated by IdentifiedNode #1695

Merged

aucampia reviewed Feb 22, 2022

View reviewed changes

ajnelson-nist and others added 2 commits February 22, 2022 18:37

Merge pull request #14 from aucampia/iwana-20220222T2241-identifiedno…

3dd2907

…de_changes Fix type checking errors

Remove unused type ignores

81a8325

ajnelson-nist marked this pull request as ready for review February 23, 2022 00:26

aucampia added the review wanted This indicates that the PR is ready for review label Feb 23, 2022

[pre-commit.ci] auto fixes from pre-commit.com hooks

06feef0

for more information, see https://pre-commit.ci

aucampia approved these changes Feb 23, 2022

View reviewed changes

aucampia requested review from ashleysommer, nicholascar and white-gecko February 23, 2022 09:57

aucampia merged commit 80a4633 into RDFLib:master Mar 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Apply IdentifiedNode to Graph iterators #1697

Apply IdentifiedNode to Graph iterators #1697

Uh oh!

ajnelson-nist commented Jan 25, 2022

Uh oh!

ajnelson-nist commented Jan 25, 2022

Uh oh!

aucampia commented Feb 20, 2022

Uh oh!

aucampia Feb 22, 2022

Uh oh!

aucampia commented Feb 22, 2022 •

edited

Loading

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

aucampia left a comment

Uh oh!

Uh oh!

	else:
	for (_s, _p, _o), cg in self.__store.triples((s, p, o), context=self):
	yield _s, _p, _o

Apply IdentifiedNode to Graph iterators #1697

Apply IdentifiedNode to Graph iterators #1697

Uh oh!

Conversation

ajnelson-nist commented Jan 25, 2022

Proposed Changes

Uh oh!

ajnelson-nist commented Jan 25, 2022

Uh oh!

aucampia commented Feb 20, 2022

Uh oh!

aucampia Feb 22, 2022

Choose a reason for hiding this comment

Uh oh!

aucampia commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

ajnelson-nist commented Feb 23, 2022

Uh oh!

aucampia commented Feb 23, 2022

Uh oh!

aucampia left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aucampia commented Feb 22, 2022 •

edited

Loading