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

Made ClosedNamespace (and _RDFNamespace) inherit from Namespace #551

Merged
merged 1 commit into from
Nov 28, 2015

Conversation

gromgull
Copy link
Member

This fixes #542

@joernhees
Copy link
Member

ERROR: Failure: RuntimeError (maximum recursion depth exceeded in cmp)

@joernhees joernhees added this to the rdflib 4.2.2 milestone Nov 22, 2015
@gromgull gromgull force-pushed the closednamedspace-inheritance-fix branch from 7d4f738 to bccc761 Compare November 22, 2015 17:02
@gromgull
Copy link
Member Author

Now only the test broken by SPARQLWrapper unicode thingy breaks :)

@property
def title(self):
return URIRef(self + 'title')

Copy link
Member

Choose a reason for hiding this comment

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

what was that for anyhow?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea, nothing broke when I removed it :)

Copy link
Member

Choose a reason for hiding this comment

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

I think it was to make this:

>>> from rdflib.namespace import DCTERMS
>>> DCTERMS.title

result in:

rdflib.term.URIRef(u'http://purl.org/dc/terms/title')

instead of:

u'Http://Purl.Org/Dc/Terms/'

Copy link
Member

Choose a reason for hiding this comment

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

So it should be added back (along with a test) since existing code will break otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

will do... too quick with that one...

Copy link
Member

Choose a reason for hiding this comment

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

Note: the title method won't be called (of course), but the value will be <built-in method upper of Namespace object at ...> instead of (e.g.) dcterms:title.

(This is a general gotcha with the Namespace utility colliding with unicode method names of course, and I guess it would be more ideal if it used __getattribute__ to never return bound methods.)

Copy link
Member

Choose a reason for hiding this comment

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

it's to override this:

In [7]: unicode('foo').title()
Out[7]: u'Foo'

Copy link
Member

Choose a reason for hiding this comment

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

maybe we should just not subclass unicode?

Copy link
Member

Choose a reason for hiding this comment

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

for now added it back + test for DCTERMS.title... in master already

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks! I jotted down the other thoughts in #556.

joernhees added a commit that referenced this pull request Nov 28, 2015
Made ClosedNamespace (and _RDFNamespace) inherit from Namespace
@joernhees joernhees merged commit 0d79d7a into master Nov 28, 2015
@joernhees joernhees deleted the closednamedspace-inheritance-fix branch November 28, 2015 11:57
@joernhees joernhees restored the closednamedspace-inheritance-fix branch November 28, 2015 12:08
@joernhees
Copy link
Member

patched with ae1f639 to allow DCTERMS.title

@gromgull
Copy link
Member Author

"Fixing" this actually broke a bit of random code for me - I used to rely on ClosedNamespace.title not returning a method. We should not ship this in a non-major release :)

@joernhees
Copy link
Member

@gromgull i don't understand, it doesn't for me:

In [1]: import rdflib
INFO:rdflib:RDFLib Version: 4.2.2-dev

In [2]: rdflib.namespace.DCTERMS
Out[2]: Namespace(u'http://purl.org/dc/terms/')

In [3]: rdflib.namespace.DCTERMS.title
Out[3]: rdflib.term.URIRef(u'http://purl.org/dc/terms/title')

@gromgull
Copy link
Member Author

The problem comes from ClosedNamespace:

In [117]: c=ClosedNamespace('http://example.org/','count')

In [118]: c.count
Out[118]: <function count>

In [119]: :(

previously this worked, since ClosedNamespace did not inherit from unicode

@joernhees
Copy link
Member

ok, i'll roll this back for the 4.2.2 release and move it to 5.0.0

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
3 participants