-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
splitdrive under windows #19695
splitdrive under windows #19695
Conversation
Note that you typically do not need to close a PR and open a new one for the same bug fix or feature. Instead you can push new commits to the same branch that the original PR is from and the PR will then incorporate those new commits. |
@@ -68,6 +68,11 @@ for S in (String, GenericString) | |||
@test joinpath(splitdir(S(homedir()))...) == homedir() | |||
@test string(splitdrive(S(homedir()))...) == homedir() | |||
|
|||
if is_windows() | |||
@test splitdrive("\\\\servername\\hello.world\\filename.ext") == ("\\\\servername\\hello.world","\\filename.ext") | |||
@test splitdrive("\\\\servername.com\\hello.world\\filename.ext") == ("\\\\servername.com\\hello.world","\\filename.ext") |
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.
probably good to also have a C:\foo\bar
test
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.
should also wrap long lines, putting the rhs of the == on the next line would work well here
Hi i have made the suggested changes. Thanks, |
Seems like this is a bugfix that should be backported to 0.5. |
@@ -68,6 +68,15 @@ for S in (String, GenericString) | |||
@test joinpath(splitdir(S(homedir()))...) == homedir() | |||
@test string(splitdrive(S(homedir()))...) == homedir() | |||
|
|||
if is_windows() | |||
@test splitdrive("\\\\servername\\hello.world\\filename.ext") == |
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.
this is inside a loop over S
but you aren't using S
like all the other tests here do (testing either String or GenericString as an approximation of unicode handling)
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.
fair point. moved tests outside the loop over S
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.
would have also worked to change the test cases to S("...")
* splitdrive under windows * splitdrive under windows tests * more test * move tests outside loop over S * changed test cases to S("...")
* splitdrive under windows * splitdrive under windows tests * more test * move tests outside loop over S * changed test cases to S("...") (cherry picked from commit 7648ad6)
This PR makes three improvements to the Windows `splitdrive` implementation: 1. The matched regex is split into pieces and annotated. 2. Forward and backward slashes are considered equivalent. This fixes #38492. 3. The patterns in the regex are reordered so that long UNC paths and long drive letters are once more recognized. This has been broken since #19695 (Julia 0.5). Co-authored-by: Jameson Nash <vtjnash@gmail.com>
Windows paths can contain non alphanumeric characters
See: https://discourse.julialang.org/t/splitdrive-under-windows/1083
@vtjnash suggested trying a PR.