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

Namespace and ClosedNamespace no longer inherit from unicode, closes #596, #542 #597

Conversation

joernhees
Copy link
Member

as mentioned in #595 and #596 this moves #551 and ae1f639 to the 5.0.0-dev branch and if accepted closes #542.

additionally as discussed removes the inheritance from unicode, meaning that all implicit string-ops like n.startswith(...) and n + ... are no longer supported

@joernhees joernhees self-assigned this Feb 15, 2016
@joernhees joernhees added this to the rdflib 5.0.0 milestone Feb 15, 2016
@osma
Copy link

osma commented Feb 15, 2016

Does this also mean that I cannot use uri.startswith(ns) anymore? As in

DCT = Namespace('http://purl.org/dc/terms/')
u = URIRef(something)

if u.startswith(DCT):
    print "yes this URI is from DC Terms"

This would break some of my code (e.g. Skosify uses this pattern quite a lot) so I'd like to know.

@joernhees
Copy link
Member Author

for now it would mean that once rdflib 5.0.0 is released, yes... (4.2.2 will definitely not change this)

but maybe we finally found a reason why we should actually also keep inheriting Namespaces from unicode as long as URIRef does... @gromgull @niklasl ?

@joernhees joernhees changed the title Namespace and ClosedNamespace no longer inherit from unicode, closes #595, #542 Namespace and ClosedNamespace no longer inherit from unicode, closes #596, #542 Feb 15, 2016
@joernhees joernhees force-pushed the replay_closednamespace_inheritance_fix branch from d0e2e9b to 1ce951b Compare February 15, 2016 18:45
@osma
Copy link

osma commented Feb 15, 2016

I don't mind if this happens in 5.0.0 since there seems to be good reasons for a) making ClosedNamespace inherit from Namespace and b) not making ClosedNamespace inherit from unicode.

But I'd propose implementing an alternative facility for 5.0.0 that can be used to check whether a URIRef belongs to a specific namespace or not. Maybe even using the uri in ns operator? For ClosedNamespace, it could match against the specific localnames that are defined as valid; for Namespace it would basically just use startswith on the underlying URI.

@gromgull
Copy link
Member

I really like using __contains__ for this! Good idea @osma !

Also, Joern, you say we can no longer use ns+localname - but we could of course implement __add__ ...

@osma
Copy link

osma commented Feb 15, 2016

Bonus points for implementing the same method (e.g. the __contains__ pattern) already in a 4.x patch release if it seems unlikely to break things. Then the same application code could work against recent rdflib 4.x versions as well as 5.0 and there would be a longer transition period instead of a sudden API change.

@niklasl
Copy link
Member

niklasl commented Feb 15, 2016

I agree, this sounds like a good way forward!

Perhaps in 4.x, ClosedNamespace could inherit from Namespace (and hence unicode), but define a __getattribute__ method to prevent attribute access from ever reaching the base class?

@joernhees
Copy link
Member Author

@gromgull that's exactly what i'm asking... to what degree should we re-implement various string methods for URIRef and Namespace? At some point it's more practical to actually keep inheriting from unicode :-/

for uri.startswith(ns) we'd need to override URIRef.startswith which is currently inherited from unicode for example, for + Namespace.__add__...

@osma we use http://semver.org so 4.2.2 should definitely be backwards compatible... in 4.2.1 Namespace actually inherits from unicode which already has a __contains__ with different semantics... (and i just reverted changes which we though "weren't gonna break anything" ;) )

@niklasl hmm, as we already tried making it saner without breaking things and failed, i'd like to just not touch that code in 4.2.2 anymore ;) People could have relied on if isinstance(ns, Namespace) not being true for ClosedNamespace and stuff like that...

I really think the cleanup belongs in 5.0.0 as it's a new major release and ok to break backwards compatibility to some degree... only question is the base-class... object or unicode?

What do we want to do in the new version without thinking about the past? Here's what i think:

  • uri.startswith(ns) (use uri in ns instead, otherwise we'd need to modify URIRef.startswith)
  • uri in ns
  • ns + localpart (could use ns[localpart] instead), but ok convenient... could add __add__

@joernhees joernhees force-pushed the replay_closednamespace_inheritance_fix branch from 1ce951b to 2b5528e Compare February 15, 2016 19:33
@gromgull
Copy link
Member

to what degree should we re-implement various string methods for URIRef and Namespace? At some point it's more practical to actually keep inheriting from unicode :-/

for uri.startswith(ns) we'd need to override URIRef.startswith which is currently inherited from unicode for example, for + Namespace.add...

I would not override any named methods, but perhaps some operators, like in / __contains__ or + / __add__ (but that one is already covered by getitem

I agree with Joern, anything that changes the API, i.e. changes contains or the class-hierarchy should wait.

@osma
Copy link

osma commented Feb 15, 2016

@joernhees @gromgull Agreed, 4.2.2 should not touch these, not even for forward compatibility. I can see now that overriding __contains__ in Namespace could cause problems for a patch release.

Also agree with implementing Namespace.__contains__ and Namespace.__add__ in 5.0.

Not sure about overriding URIRef.__contains__ in 5.0, it would ease my life a little bit but I could live with transitioning to the uri in ns pattern, which is nicer anyway.

In Skosify, I've tried to remain compatible with many different rdflib versions all the way to 2.4.x (because that used to be what was available in distribution packages), so if I want to keep that compatibility, I may have to use some kind of version-sensitive wrapper for this, but no big deal.

@joernhees joernhees force-pushed the replay_closednamespace_inheritance_fix branch from 2b5528e to 1f2461e Compare February 16, 2016 10:27
@joernhees
Copy link
Member Author

@osma you can keep compatibility by using unicode(uri).startswith(unicode(ns))... that should always work, even if we might at some point change URIRef to not be a subclass of unicode anymore (not with 5.0.0 though...)

@gromgull @niklasl i noticed a mistake wrt. IRIs in the 85227db (i used str(ns) instead of unicode(ns), which might cause trouble with IRIs... fixed that in 244007f, rebased and also made in and + work as syntactic sugar... ok to merge?

@osma
Copy link

osma commented Feb 16, 2016

@joernhees Indeed, that pattern should work both for old and new versions. Thanks for the tip!

@gromgull gromgull changed the base branch from 5.0.0-dev to master January 30, 2017 16:18
@gromgull
Copy link
Member

I made a half-assed attempt at rebasing this onto master: https://github.com/gromgull/rdflib/tree/replay_closednamespace_inheritance_fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistency Namespace vs. ClosedNamespace
5 participants