-
Notifications
You must be signed in to change notification settings - Fork 83
Introduce robust URL to java.nio.file.Path conversion #691
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
base: master
Are you sure you want to change the base?
Conversation
| * | ||
| * @since 3.20 | ||
| */ | ||
| public static Path toFilePath(URL url) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That the name contains File and Path seems to be a duplication at first, but since any kind of URL can have a path I think it should be made clear that it's a file-system path (although nio-Path can also handle non-classical file-systems).
On the other hand toFileSystemPath() seemed to be a bit verbose to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only food for thought, this suggestion isn't common convention. Could use Path fromFileUrl(URL url).
Cannot say I like that, but it seems to make it clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm torn here as well. Currently the URIUtil class has both kind of methods: toXYZ() and fromXYZ().
On one hand, it's clear from the method signature that a file-system Path is returned so we could reflect the file-URL requirement in the name. On the other hand, since this class is named URIUtil, just the method name fromFileUrl() could imply that a URI is created (which is indeed the case for the fromString() method).
A 'full' name could be filePathFromFileURL() or just pathFromFileURL() but I fear the latter misses the 'file-system' aspect and both are quite lengthy.
Maybe a different style like fileURLToPath() would also fit?
| if (!SCHEME_FILE.equals(url.getProtocol())) { | ||
| throw new IllegalArgumentException("Not a 'file' url: " + url); //$NON-NLS-1$ | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively this method could return an empty optional if the URL is not a file-URL.
But since callers that are able to handle other protocols can simple check the precondition before calling this method, I think it's fine to keep it simple for those cases where a file-URL is used with certainty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional seem much nicer, calles can throw exception if the want anyways with Optional.orElseThrow(...)
Then we can easily handel other error conditions here as well for example if invalid characters are in the path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some cases it's indeed nicer, but there are many cases where the callers have already checked or are sure that it's a file-URL and then it's more cumbersome.
Furthermore, if we check more error cases where using an optional erases detailed error information as one just gets an empty optional without more context. Using exceptions instead allows us to add more details.
d9b3e1d to
940bc52
Compare
and use it. Relates to - eclipse-platform/eclipse.platform#35
940bc52 to
b823f1d
Compare
Why not have a new |
Converting a URL into a
PathorFileis not a trivial task, especially since URLs are often malformed and therefore the straight forward wayPath.of(url.toURI())ornew File(url.toURI())lead to aURISyntaxExceptionbeing thrown. Therefore multiple different workarounds are used throughout the SDK, e.g.Path.of(url.getPath())orPath.of(url.getFile()), but they are again have their problems.This PR suggest a new utility method in the
URIUtilthat is capable of all the different conversion methods and workarounds and has the goal to reliably convertfile-URLs intojava.nio.file.Paths regardless of if the URL is malformed or perfectly complying to the standard.This could also help for eclipse-platform/eclipse.platform#35 because one does not have to rely on malformed URLs anymore if they are just passed around internally (URLs displayed to the user are another topic).