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

add convenience helper function for constructing file:/// URIs #60

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented May 1, 2018

@codecov-io
Copy link

codecov-io commented May 1, 2018

Codecov Report

Merging #60 into master will decrease coverage by 1.16%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #60      +/-   ##
==========================================
- Coverage   86.78%   85.61%   -1.17%     
==========================================
  Files           5        5              
  Lines         280      292      +12     
==========================================
+ Hits          243      250       +7     
- Misses         37       42       +5
Impacted Files Coverage Δ
src/URIParser.jl 100% <ø> (ø) ⬆️
src/parser.jl 85.71% <58.33%> (-1.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18947a0...e0ac993. Read the comment docs.

@davidanthoff
Copy link
Contributor

There is a similar effort underway at rofinn/FilePaths.jl#22. I think that would almost be cleaner, the syntax then might be URI(cwd()) and URI(p"mypath/foo.txt"). I'll try to move that stuff forward.

@vtjnash
Copy link
Member Author

vtjnash commented May 7, 2018

Similar, but this one is a bit more powerful: FilePaths can only represent the subset of URI that exist on a filesystem. This one will resolve any relative URI against some base location in the local file system. That means the relative bit can contain extra info in the form of query strings and fragments.

@davidanthoff
Copy link
Contributor

Are query strings and fragments allowed in the file URI scheme? This seems to suggest not?

@vtjnash
Copy link
Member Author

vtjnash commented May 7, 2018

That RFC appears to have been very recently proposed. The current internet standard document for URI objects (https://tools.ietf.org/html/rfc3986) doesn't specify dependence of the parse syntax being dependent on the scheme being some specific string.

@davidanthoff
Copy link
Contributor

Well, we are following some guidelines on how to convert a path to an URI. RFC3986 doesn't say anything about that, so aren't we implicitly following either rfc1738 or the new rfc8089 (which seems quite reasonable)? Neither of which seems to support fragments/queries.

@vtjnash
Copy link
Member Author

vtjnash commented May 7, 2018

Neither of those are standards though. RFC3986 is a standard, and includes explicit guidance on the meaning and parsing of a relative URI, how to combine it with a base URI, mentions some guidance on its applicability to the file: scheme, and states that all URI support fragments and queries (with sample parsers). RFC3986 additionally states (in the introduction and appendix A) that it is a clarification of RFC3986 and inherits the general syntax from that standard. It even clarifies that the syntax proposed in RFC1738 was never adopted as a standard due to the lack of ? and # being reserved as special characters and references that the file:// parser derives from specifications that includes those. What additional guidelines are you hoping for?

@davidanthoff
Copy link
Contributor

I think an alternative to this PR might be the combination of rofinn/FilePathsBase.jl#1 and rofinn/FilePaths.jl#28, or some extension of those.

@vtjnash
Copy link
Member Author

vtjnash commented Jun 1, 2018

Bump. Updated this with code to handle the drive-spec on Win32 (where the translation is defined, according to rfc8089)

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.

3 participants