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

handle encoding of base64Binary Literals #1258

Merged
merged 2 commits into from
May 15, 2021

Conversation

delocalizer
Copy link
Contributor

Fixes #1257

Proposed Changes

  • add base64 encoding for base64Binary datatype, by analogy to hexlify/unhexlify for hexBinary
  • (cosmetic) remove use of redundant lambda when handling decode

tests pass

@coveralls
Copy link

coveralls commented Feb 19, 2021

Coverage Status

Coverage decreased (-0.006%) to 75.486% when pulling 31a3d16 on delocalizer:fix/base64Binary into a8d435b on RDFLib:master.

@white-gecko
Copy link
Member

white-gecko commented Feb 19, 2021

Thank you very much for your contribution :-)

This is a case I have not encountered so far. Could you please provide a test that shows what was failing before and that it is fixed now?

@white-gecko
Copy link
Member

Will there be any interference with #1222 ?

@delocalizer
Copy link
Contributor Author

The test is by analogy with the existing test_hex_binary (minus the integer stuff that relies on hex-specific features of format builtin and int constructor)
it fails on master but passes with this PR

@delocalizer
Copy link
Contributor Author

Will there be any interference with #1222 ?

I don't believe so, the issue is not with the serialization per se, rather that encoding was not defined for xsd:base64Binary type so the decoded value was being used as the lexical. Now I think about it, a more descriptive summary of this PR might be "make handling of xsd:base64Binary type consistent with the existing handling of xsd:hexBinary."

@aucampia
Copy link
Member

@white-gecko @ashleysommer this PR is still pending on your reviews

@nicholascar
Copy link
Member

This PR has had a couple of reviews, passes all tests and introduces more tests, so I'm merging. It's pretty low-risk PR too (just an updated Literal type).

@nicholascar nicholascar merged commit cb30688 into RDFLib:master May 15, 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.

Literals with xsd:base64Binary datatype serialize incorrectly
5 participants