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

Fix serialize with multiple disks on windows #1172

Merged
merged 2 commits into from
Oct 1, 2020

Conversation

urbanmatthias
Copy link
Contributor

Fixes #1170

Proposed Changes

I replaced the path variable in the shutil.move call with location.
The former is the output of an urlencode call, which strips the drive letter on windows machines.
If the user has multiple drives, shutil.move will fail if the drive letter is not there.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 75.694% when pulling 81d0f99 on urbanmatthias:patch-1 into e56df6e on RDFLib:master.

@coveralls
Copy link

coveralls commented Sep 28, 2020

Coverage Status

Coverage decreased (-0.006%) to 75.667% when pulling 1fd1fb5 on urbanmatthias:patch-1 into e56df6e on RDFLib:master.

@ashleysommer
Copy link
Contributor

Hi @urbanmatthias
I don't think this fix is quite complete, because it will now fail for UNC file locations, which used to work, like:
file:///tmp/outfile.ttl
In that kind of string, scheme is "file" and path is /tmp/outfile.ttl so path would be the correct variable to use.

Also, just a note, this works with Windows paths too:
file://c:\\tmp\\outfile.ttl
In this example, scheme is "file" and path is c:\\tmp\\outfile.ttl so again path is the correct variable to use.

A good middleground for this fix might be to test if scheme == "file" then use path otherwise, use location.

rdflib/graph.py Show resolved Hide resolved
rdflib/graph.py Outdated Show resolved Hide resolved
rdflib/graph.py Outdated Show resolved Hide resolved
Co-authored-by: Ashley Sommer <ashleysommer@gmail.com>
@urbanmatthias
Copy link
Contributor Author

Yes you are right, thanks. I applied your suggested changes.

@ashleysommer
Copy link
Contributor

Need review from one more Maintainer before we can merge.

Copy link
Member

@nicholascar nicholascar left a comment

Choose a reason for hiding this comment

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

Looks OK to me. I folowed back through graph.py to check all the URL parsing etc. and I think this will work fine.

@ashleysommer
Copy link
Contributor

@nicholascar
As far as I can tell, this is only used in the graph.serialize() method and only when you've set a "destination" location. So unlikely to affect anything else in graph.py or URL parsing.
I'll merge.

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.

serialize fails on windows machine with multiple drives
5 participants