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

Literal.toPython() support for xsd:hexBinary #388

Closed
wants to merge 5 commits into from
Closed

Literal.toPython() support for xsd:hexBinary #388

wants to merge 5 commits into from

Conversation

bcogrel
Copy link
Contributor

@bcogrel bcogrel commented Apr 30, 2014

Following the discussion #386, here is an implementation of the bytes option, where toPython() returns a bytes value.

@bcogrel
Copy link
Contributor Author

bcogrel commented May 1, 2014

Now passing all the tests except one due to the nose last release nose-devs/nose#780 . This regression seems to be independent of this pull request and should affect the master branch as well. I was able to reproduce it and to prevent it by downgrading nose to 1.3.0.

@joernhees
Copy link
Member

could you rebase this on top of master, should've fixed the problem with nose...

(time, (lambda i:i.isoformat(), _XSD_TIME)),
(xml.dom.minidom.Document, (_writeXML, _RDF_XMLLITERAL)),
# Specific first
((basestring, _XSD_HEXBINARY), (hexlify, _XSD_HEXBINARY)),
Copy link
Member

Choose a reason for hiding this comment

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

why can't this just be (basestring, (hexlify, _XSD_HEXBINARY)),?

i guess i'll have to think about this a bit more...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point was to distinguish a regular string from a hexstring with the xsd:hexBinary datatype.

In this implementation I put specific rules first, before generic rules. The distinction between specific and generic rules is based on the datatype. Please note that the ordering of these rules is important.

We could use the more compressed structure you propose (and change _castPythonToLiteral() accordingly) if we consider that no the function _castPythonToLiteral() should never return a default datatype for a regular string.

Indeed, the rule (basestring, (None, _XSD_STRING)) would not be generic anymore while in with my approach ((basestring, None), (None, _XSD_STRING)) is still generic.

I think it is better to keep the distinction between the incoming and outcoming datatypes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 7ea1383 on bcogrel:hexstring-bytes into ad097cc on RDFLib:master.

@bcogrel
Copy link
Contributor Author

bcogrel commented May 4, 2014

I rebased it and now all tests passed. Thanks

@gromgull
Copy link
Member

gromgull commented May 7, 2014

This looks more or less ok to me - probably there will be other cases where it may be useful to have a special lexical form constructed based on a datatype. For example, you could write a rule for casting a datetime to just a date - slightly contrived example:

from datetime import datetime
now = datetime.now()
l = Literal(now, datatype=XSD.date) 

Anyone else? Any reason not to make the literal-mapping rules more powerful?

You'll have to update the bind method as well though: https://github.com/RDFLib/rdflib/blob/master/rdflib/term.py#L1493

and this: https://github.com/RDFLib/rdflib/blob/master/examples/custom_datatype.py

@bcogrel
Copy link
Contributor Author

bcogrel commented May 8, 2014

Because of the bind() function, I think it is better to separate generic and datatype-specific rules: these lists wouldn't have to be ordered anymore so adding new rules at runtime would be easier.

For backward compatibility, I propose to change the bind() prototype as follows:

def bind(datatype, pythontype, constructor=None, lexicalizer=None, datatype_specific=False)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling 3e02afe on bcogrel:hexstring-bytes into ad097cc on RDFLib:master.

@bcogrel
Copy link
Contributor Author

bcogrel commented May 8, 2014

What should I change in the custom datatype example?
https://github.com/RDFLib/rdflib/blob/master/examples/custom_datatype.py

To me, it is fine, it just shows a simple usage.

@gromgull gromgull added this to the rdflib 5.0.0 milestone Jan 12, 2017
@nicholascar
Copy link
Member

This PR has been re-issued in PR #979 to cater for Python 2/3 differences and since the original repo has gone away.

@nicholascar nicholascar mentioned this pull request Mar 19, 2020
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.

5 participants