fix: make rdflib.term.Node abstract (fixes #2518)

[lukasmu]

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.

[coveralls]

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

[aucampia]

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.

[aucampia]

@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.

[ajnelson-nist]

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: ...

[ajnelson-nist]

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

[ajnelson-nist]

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.

[lukasmu]

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.

[ajnelson-nist]

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.

[ajnelson-nist]

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.

[aucampia]

Thank you very much for having a look @ajnelson-nist - and thanks again for the fix @lukasmu