if query result source has no encoding set, fall back to utf-8 encoding. #367

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Member

gweis commented Mar 3, 2014

It's a bit tricky to do this properly for python 2 and 3, but I think this patch goes into the right direction.
For later on I think rdflib should be clear about what source is, and what attributes, methods and return values are expected.
If that would be defined, then a user of the library would know when to wrap a source within a codec or not.
resolves #344

Member

gweis commented Mar 3, 2014

Not quite sure about it, but the failing travis tests seem to be unrelated.
I hope this patch can go into 4.1.1 release, if this works for everyone

And there is at least one scenario where this code is still problematic. The decoder will fail if you pass in a StringIO instance returning unicode characters. Otherwise I think it covers a few more use scenarios than before.

Owner

joernhees commented Mar 3, 2014

yupp, sorry for the failures, master currently is broken, but fixed soon...

http://stackoverflow.com/questions/4528869/how-do-you-attach-a-new-pull-request-to-an-existing-issue-on-github

Member

gweis commented Mar 3, 2014

Hi,

Yes, read that stackoverflow, but had troubles installing the hub tool.
(Some conflict with another library I use)

However due to the resolves reference to issue #344, accepting this pull
request should also close the other issue. (Or at least mark as resolved.)

Good enough?

Cheers

Gerhard

Sent from my iPad

On 3 Mar 2014, at 7:25 pm, "Jörn Hees" notifications@github.com wrote:

yupp, sorry for the failures, master currently is broken, but fixed soon...

http://stackoverflow.com/questions/4528869/how-do-you-attach-a-new-pull-request-to-an-existing-issue-on-github


Reply to this email directly or view it on
GitHubhttps://github.com/RDFLib/rdflib/pull/367#issuecomment-36493165
.

Owner

joernhees commented Mar 3, 2014

sure, i was just confused about all the notifications ;)

From my POV the fix seems ok

Member

gweis commented Mar 3, 2014

Yeah, tried to attach the pull request to issue #344, that's why I created
and closed the ones that didn't work properly.

Sorry for the noise in the tracker.

Sent from my iPad

On 3 Mar 2014, at 9:28 pm, "Jörn Hees" notifications@github.com wrote:

sure, i was just confused about all the notifications ;)

From my POV the fix seems ok


Reply to this email directly or view it on
GitHubhttps://github.com/RDFLib/rdflib/pull/367#issuecomment-36501751
.

Owner

gromgull commented Mar 3, 2014

Did you try the trick with reading a zero length string and seeing if you get unicode or bytes back?

Owner

gromgull commented Mar 3, 2014

it may be a more robust heuristic

Owner

gromgull commented Mar 3, 2014

otherwise it looks ok to me! The StringIO from a BytesIO was ugly anyway :)

Member

gweis commented Mar 3, 2014

No, haven't thought about reading zero length string. Might fix the problem with StringIO returning unicode. I'll give it a try.

Member

gweis commented Mar 4, 2014

How about this variant? The tests pass for me.

If no one sees a problem with this variant, I'll squash the commits when merging.

Coverage Status

Changes Unknown when pulling 2498360 on gweis:master into * on RDFLib:master*.

Owner

gromgull commented Mar 4, 2014

Looks good! Squash and merge!

Member

gweis commented Mar 4, 2014

squashed into commit b7fa8d6

@gweis gweis closed this Mar 4, 2014

Owner

gromgull commented Mar 4, 2014

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment