fix: issues with string destination handling in {Graph,Result}.serialize
#2065
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5cd02f4 to
3ed412d
Compare
{Graph,Result}.serialize
3ed412d to
b53be4b
Compare
|
Not sure what is up with coverage, I'm guessing it is somehow related to the latest build on master failing (https://github.com/RDFLib/rdflib/actions?query=branch%3Amaster), I will rebase once #2068 is merged. |
b53be4b to
ea6bd62
Compare
Nevermind, it is because I fiddled with tox.ini when I was trying to debug this. |
72abe6d to
00a9167
Compare
00a9167 to
4e90415
Compare
…lize`
Change `{Graph,Result}.serialize` to only handle string destinations as URIs if
the schema is `file` and to treat it as operating system paths in all
other cases.
This is for the following reasons:
- `Result.serialize` was treating URI paths as OS paths which only works
for some cases, for special charachters and percentage encoding it
does not work.
- Many valid Unix and Windows paths parse using `urlparse` and have no netloc, e.g. `C:\some\path`
and `some:/path`, however they should be treated as paths and not as URIs.
- `Graph` and `Result` should behave consistently.
Some caveats in this change:
- non-file URIs will now be treated as OS paths which may result in
slightly confusing error messages, such as
`FileNotFoundError: [Errno 2] No such file or directory: 'http://example.com/'`
if http://example.com/ is passed.
- some valid file URIs (e.g. `file:/path/to/file` from https://datatracker.ietf.org/doc/html/rfc8089)
are also valid Posix paths but will be treated as file-URIs instead of
Posix paths. For `Graph.serialize` this ambiguity can be avoided by
using `pathlib.Path`, but for `Result.serialize` there is currently no
way to avoid it, though I will work on RDFLib#1834
soon and in that provide a way to avoid the ambiguity there also.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of changes
Change
{Graph,Result}.serializeto only handle string destinations as URIs ifthe schema is
fileand to treat it as operating system paths in allother cases.
This is for the following reasons:
Result.serializewas treating URI paths as OS paths which only worksfor some cases, for special charachters and percentage encoding it
does not work.
urlparseand have no netloc, e.g.C:\some\pathand
some:/path, however they should be treated as paths and not as URIs.GraphandResultshould behave consistently.Some caveats in this change:
slightly confusing error messages, such as
FileNotFoundError: [Errno 2] No such file or directory: 'http://example.com/'if http://example.com/ is passed.
file:/path/to/filefrom https://datatracker.ietf.org/doc/html/rfc8089)are also valid Posix paths but will be treated as file-URIs instead of
Posix paths. For
Graph.serializethis ambiguity can be avoided byusing
pathlib.Path, but forResult.serializethere is currently noway to avoid it, though I will work on Make serialize() on a CONSTRUCT result act like normal g.serialize() #1834
soon and in that provide a way to avoid the ambiguity there also.
Result.serialize()path handling is broken for windows paths and some other cases #2067Checklist
the same change.
CHANGELOG.md).so maintainers can fix minor issues and keep your PR up to date.