fix: make rdflib.term.Node abstract (fixes #2518) by lukasmu · Pull Request #2520 · 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 Commits2 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 }})

Summary of changes

Attempt to make rdflib.term.Node as well as rdflib.term.Identifier and rdflib.term.IdentifiedNode abstract base classes.
This would fix #2518. Due to this change a few type: ignore comments can be removed from the main library. However, three type: ignore comments also need to be added to a test file (though this seems reasonable in the specific case).

Checklist

Checked that there aren't other open pull requests for
the same change.
Checked that all tests and type checking passes.
If the change adds new features or changes the RDFLib public API:
- Created an issue to discuss the change and get in-principle agreement.
- Considered adding an example in ./examples.
If the change has a potential impact on users of this project:
- Added or updated tests that fail without the change.
- Updated relevant documentation to avoid inaccuracies.
- Considered adding additional documentation.
Considered granting push permissions to the PR branch,
so maintainers can fix minor issues and keep your PR up to date.

coverage: 90.933%. first build when pulling df4f5fa on lukasmu:bugfix/abstract into d163c2e on RDFLib:main.

Choose a reason for hiding this comment

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

Thanks for the PR @lukasmu - it looks very good.

I don't exactly like the n3 method though, at some point we have to try get rid of it, N3 is not ratified by W3C and is not really using the RDF data model, so having a method named after it is a bit odd, and it also does very different things for Graph and for Identifier.

That being said, this PR essentially just documents the facts as they are in the type system, so I think it's the best option at the moment.

@ajnelson-nist if you have a moment, please have a look. The only slightly controversial thing here is to have n3 as high up in the class hierarchy as rdflib.term.Node, but I think it's justified.

@@ -1609,10 +1609,9 @@ def update(

return processor.update(update_object, initBindings, initNs, **kwargs)

def n3(self) -> str:
def n3(self, namespace_manager: Optional["NamespaceManager"] = None) -> str:

Choose a reason for hiding this comment

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

I'm a little confused. The body of this method is updated to use namespace_manager as a keyword argument on an n3() method; but, this syntax looks like it's specifying namespace_manager as a positional argument.

Could the signature become instead:

def n3( self, *args: Any, namespace_manager: Optional["NamespaceManager"]=None, **kwargs: Any, ) -> str: ...

Comment on lines -1614 to -1615

# type error: "IdentifiedNode" has no attribute "n3"
return "[%s]" % self.identifier.n3() # type: ignore[attr-defined]

Choose a reason for hiding this comment

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

I'm glad to see these no-attribute issues being addressed.

Choose a reason for hiding this comment

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

Overall, I agree with the effects of this PR. n3() method calls have been in place for some time, so this looks like overall the typing becomes more coherent.

My one request for a change is, I think, minor w.r.t. the rest of the PR: to decide whether namespace_manager is a positional or a keyword argument. The function signatures seem to be saying positional, the function calls seem to be saying keyword.

Thanks for the feedback!

When coding in Python and calling a function I always tend to provide the name of the arguments even if they are positional arguments. Just because "Explicit is better than implicit" (related https://www.youtube.com/watch?v=wf-BqAjZb8M&t=43m15s).
In Python we almost never see positional-only arguments (though they exist as of Python 3.8):

def f(positional_parameter, /, positional_or_keyword_parameter, *, keyword_parameter):
    pass

Arguments like namespace_manager are thus technically positional-or-keyword parameters.

Thanks for the feedback!

When coding in Python and calling a function I always tend to provide the name of the arguments even if they are positional arguments. Just because "Explicit is better than implicit" (related https://www.youtube.com/watch?v=wf-BqAjZb8M&t=43m15s). In Python we almost never see positional-only arguments (though they exist as of Python 3.8):
def f(positional_parameter, /, positional_or_keyword_parameter, *, keyword_parameter):
   pass
Arguments like namespace_manager are thus technically positional-or-keyword parameters.

I suppose my complaint amounts to a difference of opinion in coding style. I've often been confused over the years with function definition styles in Python slightly obscuring whether an argument was positional or a keyword, particularly when default values were used too. I became a happier programmer when I learned about the asterisk delimiter. I've avoided the / because I needed to support Python 3.7 applications, but now that that version's sunset, I'll consider it if I ever come across a good reason.

I tried to get examples in your explicit-name style to fail type review with confusion of a keyword vs positional argument, but your call style seemed to work fine. So, I'll leave it to the RDFLib developers to settle on style.

Choose a reason for hiding this comment

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

I have no blocking feedback on this PR. @aucampia , I don't think my Request Changes review prevents a merge, but if it does, and this Approval action doesn't undo it, please feel free to override it.

Labels

ready to merge

The PR will be merged soon if no further feedback is provided.

review wanted

This indicates that the PR is ready for review