Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix leading slash for abspath in joinpath #62

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

Conversation

pb866
Copy link

@pb866 pb866 commented Dec 28, 2024

Currently, joinpath gives two leading slashes, when an absolute path is joined to an empty URI path. This PR fixes this issue.

julia> using URIs

julia> u = URI("http://test.com")
URI("http://test.com")

julia> joinpath(u, "/abs/path")
URI("http://test.com//abs/path")

Signed-off-by: Peter Bräuer <pb866.git@gmail.com>
Add tests to add absolut path with and without trailing slash and
absolut path with additional part to an empty uri path.

Signed-off-by: Peter Bräuer <pb866.git@gmail.com>
@pb866
Copy link
Author

pb866 commented Jan 12, 2025

Is there anything that needs to be done before this can be merged? I think, the failing test has nothing to do with this fix.

An alternative fix would be to ensure non-empty URI paths, so the condition that was adjusted in this PR is not needed at all. This is also a workaround for the current situation (without tempering with the package's code), but it would be nice to have a proper resolution one way or the other.

@ararslan
Copy link
Member

ararslan commented Feb 20, 2025

Thanks for the contribution and apologies for the delayed response. What you've implemented here seems reasonable to me and is what I would generally have expected joinpath to do in this case. My read of RFC 2396 also suggests that what you're proposing is the correct behavior and joinpath is currently (arguably) not compliant. However, this is technically breaking as some code may be relying on the current behavior. I'm in favor of merging but would want a second opinion; @quinnj?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants