Skip to content
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

Adding Namespace.__contains__() #1237

Merged
merged 3 commits into from Feb 9, 2021

Conversation

JaimieMurdock
Copy link
Contributor

No issue for this, just a quick addition for usability/literacy while working with the library.

Proposed Changes

  • Adds a Namespace.__contains__() method to enable syntax sugar of ref in ns. This is a natural way to discover if an arbitrary URIRef is in the Namespace.
  • Current method would be manually writing ref.startswith(ns), which doesn't preserve semantics

This has possible extension to NamespaceManager, testing if a URIRef is in any managed namespace.

class NamespaceManager():
    def __contians__(self, ref):
        return any(ref.startswith(ns) for x, ns in self.namespaces())

Note that in NamespaceManager, namespaces are stored as URIRefs, not Namespace objects :/

Hoping this starts a discussion about some usability concerns.

@white-gecko
Copy link
Member

Hey @JaimieMurdock, thank you for submitting a pull-request. I understand your motivation but I'm unsure if this extra code is worth the convenience as other.startswith(self) is a single expression as well. I understand the argument for the NamespaceManagemer, but as I see it would work without the proposed change but sure would make more sense if both classes get the __contains__() method.
So I'm 50/50 for this change, maybe @nicholascar or @ashleysommer have a stringer opinion ;-)

@JaimieMurdock
Copy link
Contributor Author

I'll add the NamespaceManager.__contains__() method to a commit shortly.

The major motivator is "Readability counts." from PEP 20. Also, was the "natural" way of writing it for a relatively-new-to-Python student working on the project.

@JaimieMurdock
Copy link
Contributor Author

Starting to peel back the layers on Namespace management and there's quite a bit of counter-intuitive typing - had to add a method to ClosedNamespace and _RDFNamespace as well. I'll write up a proper Issue today.

@JaimieMurdock
Copy link
Contributor Author

Worth noting that #597 proposed the __contains__ functionality implemented here for 5.0.0.

@nicholascar
Copy link
Member

I'm happy for any considered work to make any Pythonic ways of querying RDFlib objects work. I suspect that work to implement this function, even if the use of the output is essentially the same as the startwith way of working, will address inconsistencies or old coding styles in the code base so it is worth it.

So, if you're prepared to dive into Namespace to implement stuff and the PR passes all tests, great!

@ashleysommer
Copy link
Contributor

I'll close this PR and re-open it to hopefully trigger Drone to see the PR and run its tests.

@ashleysommer ashleysommer reopened this Feb 9, 2021
@ashleysommer
Copy link
Contributor

ashleysommer commented Feb 9, 2021

Nope, looks like Drone only kicks in for new pull requests.
However the close/open did trigger the New travis config on travisci .com which does work a little bit better than the old one from travisci .org, see it here: https://travis-ci.com/github/RDFLib/rdflib/builds/216508899

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.492% when pulling ac51ae5 on JaimieMurdock:Namespace.contains into 8e3c771 on RDFLib:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.492% when pulling ac51ae5 on JaimieMurdock:Namespace.contains into 8e3c771 on RDFLib:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 75.492% when pulling ac51ae5 on JaimieMurdock:Namespace.contains into 8e3c771 on RDFLib:master.

@ashleysommer ashleysommer merged commit c11f7b5 into RDFLib:master Feb 9, 2021
@white-gecko white-gecko mentioned this pull request Feb 17, 2021
@white-gecko white-gecko added this to the rdflib 6.0.0 milestone Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants