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

Fix #31368: joinpath works on collections of paths #38263

Merged
merged 4 commits into from
Apr 22, 2021

Conversation

belamenso
Copy link
Contributor

@belamenso belamenso commented Nov 1, 2020

Implements @StefanKarpinski's proposition from #31368. joinpath(splitpath(x)) == x should now hold.

close #31368

@belamenso belamenso force-pushed the fix31368 branch 3 times, most recently from d0b092d to 10d506e Compare November 1, 2020 22:08
@StefanKarpinski
Copy link
Sponsor Member

Thanks for updating this!

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

A few tweaks to avoid the unconstrained joinpath(paths) signature and to generalize indexing so that indexed collections whose indexing doesn't start at 1 will work.

base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
@belamenso belamenso force-pushed the fix31368 branch 2 times, most recently from d913cd1 to ca8ab7d Compare November 2, 2020 18:32
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
base/path.jl Outdated
function joinpath(paths::Union{Tuple, AbstractVector})::String
assertstring(x) = x isa AbstractString || throw(ArgumentError("path component is not a string: $(repr(x))"))

isempty(paths) && return path_separator
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale for returning path_separator here? Noting that joinpath() throws, and joinpath("a") == "a", doesn't contain a path separator.

Copy link
Contributor Author

@belamenso belamenso Nov 3, 2020

Choose a reason for hiding this comment

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

When extending the interface to collections I had to pick something for [] and ().
@StefanKarpinski stated clearly in #20912 that joinpath should not throw, I treat these errors as typechecking. As to why separator:

  • Separator on both Windows and Unix systems is a correct path to the root of the current drive.
  • Empty string is not a valid path on Unix.
  • Returning the root of the current drive (/ on Unix and C:\ on Windows) requires looking at filesystem on Windows, then joinpath would behave differently depending on current working directory and would no longer be just a data manipulation function (joinpath should not throw. period. #20912 (comment)).

I don't have any better idea.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I interpret (I can be wrong!) "joinpath should not throw" as it shouldn't depend on the content of the string arguments. But joinpath() already throws (a MethodError), and I wouldn't be too worried that joinpath(()) also throws, and we keep the equivalence joinpath(a::Vector{String}) == joinpath(a...).
And you could define only one method taking string arguments as

joinpath(paths::AbstractString...)::String = joinpath(paths)

I'm not sure if I would prefer joinpath() or joinpath([]) to throw or to return something which is "neutral" for path joining, which would rather be the empty string than / (returning the empty string is not the same as throwing an error, even if the empty string is not a valid path).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some consideration and @rfourquet's comments I now support throwing here:

  • this was the original author's intent, which can be seen in missing joinpath(),
  • there doesn't seem to be any clear best candidate for return value,
  • symmetry with joinpath(AbstractString...), which is now implemented as
joinpath(paths::AbstractString...)::String = joinpath(paths),
  • joinpath doesn't seem to be used in many dynamic situations where [] might come up, it's usually applied to a pair of some directory variable and a filename string.

I modified the code accordingly.

Copy link
Member

Choose a reason for hiding this comment

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

I see that you updated the code according to my feedback, but again, my opinion is just my opinion :) Maybe @StefanKarpinski will have another take, let's wait for more feedback.

base/path.jl Outdated Show resolved Hide resolved
@belamenso belamenso force-pushed the fix31368 branch 2 times, most recently from 4bd0a8f to aac6b2a Compare November 3, 2020 16:13
if Sys.iswindows()

function joinpath(path::AbstractString, paths::AbstractString...)::String
result_drive, result_path = splitdrive(path)
function joinpath(paths::Union{Tuple, AbstractVector})::String
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not duck type this and accept any iterator instead of only tuple and AbstractVector?

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The single-argument joinpath is ambiguous between a single string-like argument which should be converted to a string and a single argument that iterates path components which should be iterated, converted to strings and joined with path separators. Suppose someone tries to call this on an exotic type that's meant to represent a string-like type that doesn't subtype AbstractString, then what? If this API was duck-typed, then it would try to treat the argument like a collection and iterate it and then join the results together as strings. That might work fine if the thing happens to iterate path components, but what if it iterates something else like individual characters or grapheme clusters (which are represented as strings)? It's much better to make this API clearly constrained without ambiguity: we know what to do if the argument is an array or a tuple. If people really want to call joinpath on other collections, we can add those in the future on an as-needed basis, but it just doesn't seem likely to be necessary.

base/path.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added forget me not PRs that one wants to make sure aren't forgotten merge me PR is reviewed. Merge when all tests are passing and removed forget me not PRs that one wants to make sure aren't forgotten labels Apr 15, 2021
base/path.jl Show resolved Hide resolved
base/path.jl Outdated Show resolved Hide resolved
@musm
Copy link
Contributor

musm commented Apr 22, 2021

This should be ready to go. Anyone else want to quickly sign off?

@musm musm merged commit fc02458 into JuliaLang:master Apr 22, 2021
@simeonschaub simeonschaub removed the merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
…g#38263)

* Fix JuliaLang#31368: joinpath works on collections of paths

* Update base/path.jl

Co-authored-by: Jameson Nash <[email protected]>

* Apply suggestions from code review

* Fixup

Co-authored-by: Mustafa M <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
…g#38263)

* Fix JuliaLang#31368: joinpath works on collections of paths

* Update base/path.jl

Co-authored-by: Jameson Nash <[email protected]>

* Apply suggestions from code review

* Fixup

Co-authored-by: Mustafa M <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
…g#38263)

* Fix JuliaLang#31368: joinpath works on collections of paths

* Update base/path.jl

Co-authored-by: Jameson Nash <[email protected]>

* Apply suggestions from code review

* Fixup

Co-authored-by: Mustafa M <[email protected]>
Co-authored-by: Jameson Nash <[email protected]>
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.

joinpath able to handle output of splitpath
7 participants