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

joinpath & allow kwargs in constructor #12

Merged
merged 4 commits into from
Nov 24, 2020
Merged

Conversation

Roger-luo
Copy link
Contributor

@c42f I implemented the joinpath and URI mentioned in #10

test/uri.jl Show resolved Hide resolved
Copy link
Collaborator

@c42f c42f left a comment

Choose a reason for hiding this comment

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

This looks nice, thanks.

My only thought is that we should perhaps follow the lead of joinpath for posix paths, in terms of handling absolute path components.

src/URIs.jl Outdated Show resolved Hide resolved
@Roger-luo Roger-luo requested a review from c42f November 13, 2020 22:52
@Roger-luo
Copy link
Contributor Author

@c42f bump

@Roger-luo
Copy link
Contributor Author

PS. can we have a new tag after this gets merged? I kinda want to release my package with this new interface directly instead of updating later...

Copy link
Collaborator

@c42f c42f left a comment

Choose a reason for hiding this comment

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

PS. can we have a new tag after this gets merged?

Yes! Please bump to 1.1.0 :-)

src/URIs.jl Outdated Show resolved Hide resolved
if isempty(uri.path)
path = "/" * path
end
return URI(uri; path=normpath(path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rather avoid normpath as that uses system-dependent regexes to parse the path (including drive letters and such, on windows).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's follow the exact rules in RFC1808 section 4 I guess?

https://tools.ietf.org/html/rfc1808#section-4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh dear, yes indeed!

This is what happens when trying to maintain a package I seem to have inherited by accident 😆

Co-authored-by: Chris Foster <chris42f@gmail.com>
@c42f c42f merged commit 9319a78 into JuliaWeb:master Nov 24, 2020
@c42f
Copy link
Collaborator

c42f commented Nov 24, 2020

Thanks, this is very nice. I'm sure it will be useful.

@c42f
Copy link
Collaborator

c42f commented Nov 24, 2020

New version: JuliaRegistries/General#25219

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.

2 participants