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 splitpath(p::AbstractString) to Base.Filesystem. #28156

Merged
merged 16 commits into from
Sep 24, 2018

Conversation

NHDaly
Copy link
Member

@NHDaly NHDaly commented Jul 17, 2018

Opposite of joinpath(), it separates a path into its parts.

Copy link
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.

I've definitely wanted this before, so 👍. Need to figure out what to do about drive letters on Windows 😭

base/path.jl Outdated
```
"""
function splitpath(p::AbstractString)
out = ()
Copy link
Member

Choose a reason for hiding this comment

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

This should return a Vector{String} not a tuple. That way you can also push stuff onto it.

Copy link
Member Author

Choose a reason for hiding this comment

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

okay yeah, sounds good to me. i'll change that now! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@NHDaly
Copy link
Member Author

NHDaly commented Jul 17, 2018

I think that last push should fix the windows case, but i'd have to switch over to my bootcamp to really check (and/or add better windows-specific tests and wait for appveyor, but.. yeuckh)..

base/path.jl Outdated
@@ -211,13 +211,15 @@ julia> splitpath("/home/myuser/example.jl")
```
"""
function splitpath(p::AbstractString)
out = ()
out = AbstractString[]
Copy link
Member

@StefanKarpinski StefanKarpinski Jul 17, 2018

Choose a reason for hiding this comment

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

Better to make it just String[], Postel's Law and all: this should acept any string but return one specific kind of string vector, i.e. Vector{String}. I'm pretty sure dirname et al. always return String anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure dirname et al. always return String anyway.

help?> dirname
search: dirname

  dirname(path::AbstractString) -> AbstractString

That's actually the reason I made it an AbstractString, was to match dirname/basename.

... But actually those end up calling splitdir, which definitely does return a String. And, despite what the docstring says, it also only accepts a String?:

julia/base/path.jl

Lines 131 to 137 in a2fdb7d

function splitdir(path::String)
a, b = splitdrive(path)
m = match(path_dir_splitter,b)
m === nothing && return (a,b)
a = string(a, isempty(m.captures[1]) ? m.captures[2][1] : m.captures[1])
a, String(m.captures[3])
end

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I've changed my splitpath to match: Take and return String, but say AbstractString in the docs.. I want to match the style of the other functions.

If those should be changed, this can be done in the same batch. :)

@quinnj
Copy link
Member

quinnj commented Jul 17, 2018

Just leaving a reference here to HTTP.jl's splitpath implementation; it's URI-specific, but I thought I'd mention it anyway.

base/path.jl Outdated
@@ -196,6 +197,31 @@ function pathsep(paths::AbstractString...)
return path_separator
end

"""
splitpath(path::AbstractString) -> [AbstractString, AbstractString, AbstractString...]
Copy link
Member

Choose a reason for hiding this comment

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

You can write the return type as Vector{String}.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

base/path.jl Outdated
out = String[]
while true
isempty(p) && break
ismount(p) && (push!(out, dirname(p)); break)
Copy link
Member

@StefanKarpinski StefanKarpinski Jul 17, 2018

Choose a reason for hiding this comment

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

How about doing b, d = splitdir(p) on the line before this since that gets you the basename and the dirname in a single function call (otherwise it's getting computed repeatedly). Then you can change this to push!(out, d) and replace basename(p) below with b and dirname(p) with d. Path manipulation functions aren't generally performance-critical, but there's no sense in being profligate 😀

Copy link
Member Author

@NHDaly NHDaly Jul 17, 2018

Choose a reason for hiding this comment

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

Ah, yeah, thanks! :) This was left over from my original version which was mostly just trying to be line-count conservative for Slack. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

base/path.jl Outdated
push!(out, basename(p))
p = dirname(p)
end
return reverse(out)
Copy link
Member

Choose a reason for hiding this comment

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

Since this function "owns" this vector, it's safe to use reverse! to reverse it in-place. Either that or build it backwards with pushfirst! instead of push! and reverse at the end. I'm not sure which is faster (and it probably doesn't matter).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, i'm not sure which is faster either. I left it with push! and reverse (i'll change to reverse!) because i was editing in Atom and I don't have julia 0.7 working in Atom yet.. -_-

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

test/path.jl Outdated
@@ -85,6 +85,17 @@
end
@test relpath(S(joinpath("foo","bar")), S("foo")) == "bar"

@testset "splitpath" begin
@test ("a", "b") == splitpath("a/b")
Copy link
Member

Choose a reason for hiding this comment

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

You're going to need to update the tests to compare with vectors, i.e. ["a", "b"] instead of ("a", "b").

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, thanks. Good call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

base/path.jl Outdated
# Examples
```jldoctest
julia> splitpath("/home/myuser/example.jl")
("/", "home", "myuser", "example.jl")
Copy link
Member

Choose a reason for hiding this comment

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

This output is going to be wrong now that this returns an array.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks! Fixed.

base/path.jl Outdated
while true
isempty(p) && break
d, b = splitdir(p)
ismount(p) && (push!(out, d); break)
Copy link
Member

Choose a reason for hiding this comment

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

ismount is definitely not what we want here—this function should be purely a string manipulation function and ismount does an lstat on a path and checks if it's a directory whose parent directory is on a different device. I think the right behavior for Windows is to split the drive off at the start and then concatenate it back with the first element in the path at the end.

Copy link
Member Author

@NHDaly NHDaly Jul 18, 2018

Choose a reason for hiding this comment

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

Yeah you're definitely right about that. Good call. Actually, it looks like dirname already has the right drive logic for windows, so i think my original code was actually reasonable, dirname(p) == p. That is:

d == p && (push!(out, d); break)

That should be fine both for the unix and windows cases: dirname("/") returns "/" and dirname("C:") should return "C:".

Let me put it back to that and we can think more about it.


(EDIT: done)

test/path.jl Outdated
@test ["/", "a", "b"] == splitpath("/a/b")
@test ["/"] == splitpath("/")
@test ["a"] == splitpath("a/")
@test ["a", "b"] == splitpath("a/b/")
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if, in the presence of a trailing slash, the last component should be an empty string.

Copy link
Member Author

@NHDaly NHDaly Jul 18, 2018

Choose a reason for hiding this comment

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

Meep. I guess you're probably right:

julia> splitdir("a/")
>> ("a", "")
julia> splitdir("a///")
>> ("a", "")

I have never liked when utilities treat paths with a trailing slash differently, because tools seem to be inconsistent with when they add one. Haha oh, I guess I just described Postel's Law! Learned something new today. But yeah, like with Postel's Law, I think I wanted to be flexible with the input, so it would work whether or not there was a trailing slash. And I can't imagine why someone would want an empty path component in an array of the path components...

*But* I also think consistency across these functions is important, so I'm happy to go with whatever you think is best.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fwiw, python3 does not add the trailing "":

>>> from pathlib import PurePosixPath, PureWindowsPath
>>> PurePosixPath('/path/to/file.txt').parts
('/', 'path', 'to', 'file.txt')
>>>
>>> PurePosixPath('/path/to/dir/').parts
('/', 'path', 'to', 'dir')

Copy link
Member

Choose a reason for hiding this comment

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

Let's follow Python here—that's what I did for the rest of the path utils. I also think it's the right call, having thought about it some more: otherwise code that calls splitpath always has to look out for the trailing slash corner case. What was pushing me in the other direction was wanting to have some kind of identity that joinpath(splitpath(path)) == path, but I think avoiding the trailing slash gotcha is more important. That is probably why the Python function works the way it does.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah that's too bad that joinpath(splitpath(path)) can change the trailing slash....

I was going to say that actually joinpath(splitpath(path)) == normpath(path), and it almost does, except for that it always trims trailing slashes. But I think that's okay, for all the reasons we just cited above. :) Thanks!

@ararslan ararslan added the filesystem Underlying file system and functions that use it label Jul 18, 2018
@NHDaly
Copy link
Member Author

NHDaly commented Jul 18, 2018

I think the right behavior for Windows is to split the drive off at the start and then concatenate it back with the first element in the path at the end.

Take another look when you can. I think the way it is now is actually best. It's generic for both unix and windows, and, I think, simple to understand!

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jul 18, 2018

Yes, you're right—this is the way to go. I was confused by the tests which are not quite right, see this actual behavior as compared to what's in the tests:

module Windows
    module Sys
        isunix() = false
        iswindows() = true
    end
    include("base/path.jl")
end

julia> Windows.splitpath(raw"C:\a\b\c")
4-element Array{String,1}:
 "C:\\"
 "a"
 "b"
 "c"

We should really make it possible to test the Windows and UNIX pure path operations on both kinds of systems—making sure the test are right is really annoying. The module trick I used here lets you get a Windows module that has all the Windows versions of the path functions.

@NHDaly
Copy link
Member Author

NHDaly commented Jul 18, 2018

Oh cool, that's a neat trick! :)

And oh okay cool, thanks for pointing that out; I've fixed the windows tests to keep the slash in the drive name.

Also, through your test Windows module set up, I was able to see that windows handles unix-style "/" paths correctly as well, so I moved all those tests out of the else block and made them universal.

test/path.jl Outdated
@test ["J:"] == splitpath("J:\\\\")
@test ["C:\\", "a", "b", "c"] == splitpath("C:\\\\a\\b\\c")
@test ["C:\\"] == splitpath("C:\\\\")
@test ["J:\\"] == splitpath("J:\\\\")
Copy link
Member

Choose a reason for hiding this comment

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

Add a test for a relative path with a drive? (Yes, this is a thing, weirdly enough.)

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean by this? Can you give me an example?

Copy link
Member

Choose a reason for hiding this comment

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

C:, C:a, C:a\b

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

@StefanKarpinski
Copy link
Member

Nice, this is looking really good! Aside from testing relative paths with drives on Windows, this seems pretty good to go to me. It might also be good to test a few more weird corner cases: drive-only, etc.

base/path.jl Outdated
dir, base = splitdir(p)
dir == p && (push!(out, dir); break) # Reached root node.
if !isempty(base) # Skip trailing '/' in basename
push!(out, base)
Copy link
Member

Choose a reason for hiding this comment

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

pushfirst! and skip the reverse! is clearer IMO (as suggested earlier).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

base/path.jl Outdated
if !isempty(base) # Skip trailing '/' in basename
push!(out, base)
end
p = dir;
Copy link
Member

Choose a reason for hiding this comment

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

Remove trailing ;

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -122,6 +122,7 @@ precompile(Tuple{typeof(Base.Filesystem.isdir), String})
precompile(Tuple{typeof(Base.Filesystem.pwd)})
precompile(Tuple{typeof(Base.Filesystem.splitdir), String})
precompile(Tuple{typeof(Base.Filesystem.splitext), String})
precompile(Tuple{typeof(Base.Filesystem.splitpath), String})
Copy link
Member

Choose a reason for hiding this comment

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

I think this file is generated automatically so you can probably skip this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh cool, okay thanks. Removed.

test/path.jl Outdated
@@ -85,6 +85,38 @@
end
@test relpath(S(joinpath("foo","bar")), S("foo")) == "bar"

@testset "splitpath" begin
@test ["a", "b", "c"] == splitpath(joinpath("a","b","c"))
Copy link
Member

Choose a reason for hiding this comment

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

Ordering the test here and below as @test splitpath(joinpath("a","b","c")) == ["a", "b", "c"] is clearer IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks! I should've noticed that the surrounding test style is the way you have it. Old habits from my last company. :) Thanks!

@NHDaly
Copy link
Member Author

NHDaly commented Jul 18, 2018

PTAL

test/path.jl Outdated
@test splitpath("J:\\") == ["J:\\"]
@test splitpath("C:") == ["C:"]
@test splitpath("C:a") == ["C:", "a"]
@test splitpath("C:a\\b") == ["C:", "a", "b"]
Copy link
Member

@StefanKarpinski StefanKarpinski Jul 18, 2018

Choose a reason for hiding this comment

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

I'm not sure about these relative-path-with-drive cases. You can think about UNIX paths like driveless Windows paths where the drive is implicitly whatever the current drive is. Since splitpath("a") == ["a"] has only one path component, why does splitpath("C:a") have two? The drive isn't a path component at all, so this is a bit like having splitdrive("a") == ["", "a"] if we remove the optional drive part. I think that we should have the following behaviors instead:

@test splitpath("C:") == ["C:"]
@test splitpath("C:a") == ["C:a"]
@test splitpath("C:a\\b") == ["C:a", "b"]

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you, i think! I didn't really understand drives before.

Ah, okay, so the current behavior is a consequence of the way splitdir treats "C:a":

julia> Windows.dirname("C:a")
"C:"

I'm now understanding that to mean "there is no directory above a in drive C:. The result of this function call is the empty string, but in drive C:." (That is, the current directory in drive C:.) Is that right?

If so, then I agree, the result should be C:a.

To get that behavior, i think we can do what you originally recommended, which is to strip of the drive via splitdrive at the start, and then just tack it back on to the first element of the string at the end. Does that sound good to you?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that should work. Give it a try and see how the tests fare.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. This worked great! :)

test/path.jl Outdated
@@ -85,6 +85,41 @@
end
@test relpath(S(joinpath("foo","bar")), S("foo")) == "bar"

@testset "splitpath" begin
@test splitpath(joinpath("a","b","c")) == ["a", "b", "c"]
@test splitpath("") == []
Copy link
Member

Choose a reason for hiding this comment

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

This seems wrong to me. In particular, note that joinpath() is an error, not "". This seems like it should be [""] instead of []; joinpath("") == "" so you get the input back as you would want.

Copy link
Member Author

@NHDaly NHDaly Jul 18, 2018

Choose a reason for hiding this comment

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

hmm. Yeah I think that makes sense (from the identity viewpoint).

As another perspective, if I ask "what are the directories and files in this here path: ""?", then "There aren't any! That is, it's these ones: []" seems to me like the right response. Certainly it seems more plausible an answer than "ah yes, it contains this file: [""].".

Put another way, should length(splitpath("")) really be the same as length(splitpath("a.txt"))? I would want length(splitpath("")) to be 0.

So I'm not sure what's best there. I guess it seems noteworthy that you can't get splitpath to return [""], I just don't know if that's a good or a bad thing. I would say that [""] is kind of meaningless, since that array doesn't contain either a file or a directory.

That said, I do agree that having a potential error lurking inside joinpath(splitpath(p)...) seems scary...

Copy link
Member

Choose a reason for hiding this comment

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

There aren't any!

This isn't true though: "" is the path of the current directory. It has a single component which happens to be the empty string, but that's not really relevant. You can replace the empty string in these cases with . and everything should behave the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! Really? Yeah well, in that case i'm happy.

In that case, I think it does make sense that length(splitpath("")) is 1. So length(splitpath(p)) can never be 0, and I think that makes sense. Okay yeah, I'll change this then. Thanks for explaining that!

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@ararslan ararslan added the needs news A NEWS entry is required for this change label Jul 18, 2018
@NHDaly
Copy link
Member Author

NHDaly commented Jul 20, 2018

Okay, so I've got this almost working. I made the changes we discussed above, and those test cases all work.

But now I'm baffled by two new unit test errors i've run across:

julia0.7> @test Windows.splitpath("C:\\\\a\\b\\c") == ["C:\\", "a", "b", "c"]
Test Failed at REPL[18]:1
  Expression: splitpath("C:\\\\a\\b\\c") == ["C:\\", "a", "b", "c"]
   Evaluated: ["C:\\\\a\\b\\", "c"] == ["C:\\", "a", "b", "c"]
ERROR: There was an error during testing

and the corresponding non-drive example:

julia0.7> Windows.splitpath("\\\\a\\b\\c")
>> 2-element Array{String,1}:
 "\\\\a\\b\\"
 "c"

Strangely, this seems to come from the behavior or splitdir:

julia0.7> Windows.splitdir("\\\\a\\b")
>> ("\\\\a\\b", "")

But it's specific to two leading slashes (two escaped slashes):

julia0.7> Windows.splitdir("\\a\\b")
>> ("\\a", "b")

julia0.7> Windows.splitdir("\\\\a\\b")
>> ("\\\\a\\b", "")

julia0.7> Windows.splitdir("\\\\\\a\\b")
>> ("\\\\\\a", "b")

julia0.7> Windows.splitdir("\\\\\\\\a\\b")
>> ("\\\\\\\\a", "b")

Note that it's fine in non-windows:

julia0.7> splitdir("//a/b")
>> ("//a", "b")

@NHDaly
Copy link
Member Author

NHDaly commented Jul 20, 2018

Ah, okay, I've found the problem. The problem is that Windows.splitdrive somehow thinks the whole thing is a drive:

julia0.7> Windows.splitdrive("\\\\a\\b")
>> ("\\\\a\\b", "")

This looks like a bug to me.

@StefanKarpinski
Copy link
Member

@vtjnash, weren’t you complaining about these windows regexes recently?

@NHDaly
Copy link
Member Author

NHDaly commented Jul 20, 2018

Huh, I don't know whether it's a bug or not actually..
This is the part of the regex that's matching:
\\\\[^\\]+\\[^\\]+
It's the second component in the splitdrive regex, here:

m = match(r"^([^\\]+:|\\\\[^\\]+\\[^\\]+|\\\\\?\\UNC\\[^\\]+\\[^\\]+|\\\\\?\\[^\\]+:|)(.*)$", path)

... is \\a\b somehow actually a valid drivename in windows?
If so, then the reason it's doing splitpath("C:\\\\a\\b\\c") wrong is because we're essentially calling splitdrive twice; once in splitpath and once for every call to splitdir. I'll probably want to change the implementation to not call splitdrive at the beginning. One way to fix it would be to call splitdrive manually on each element and compare it with the output of splitdir, and then put it back if the only thing splitdir is doing is popping off the drive?

Probably the Most Correct thing to do would be to make another version of splitdir that skips the splitdrive part, and to use that instead of splitdir in my implementation for splitpath. What do you think?

@StefanKarpinski
Copy link
Member

Comparing with Python is probably a good way to check what’s right—they have the same API and presumably get this right.

@NHDaly
Copy link
Member Author

NHDaly commented Jul 20, 2018

:O woahhhhh

import os
>>> os.path.dirname("\\a\\b")
'\\a'
>>> os.path.dirname("\\\\a\\b")
'\\\\a\\b'

Weird, man.

>>> from pathlib import Path
>>> Path('\\\\a\\b\\c').parts
('\\\\a\\b\\', 'c')
>>> Path('C:\\\\a\\b\\c').parts
('C:\\', 'a', 'b', 'c')

Huh, so yeah, i guess we should respect the weirdness. So then yeah i think we'll have to change the way the function works, because right now, when we pull off the drivename at the beginning, it changes how the rest of the path parses. So i think probably the best change is to make a custom splitdir_ignore_drive type function that does the same thing as splitdir but without the splitdrive part. Does that sound good to you?

@NHDaly
Copy link
Member Author

NHDaly commented Jul 20, 2018

Okay, i pushed up a commit that does what i'm describing. I dunno about the naming for the added function, but this at least gives us something concrete to discuss.

All the tests should pass now! :)

@StefanKarpinski
Copy link
Member

Oh yeah, I rebased this locally and fixed some merge conflicts and then pushed it.

@StefanKarpinski
Copy link
Member

Thanks for the great work on this, @NHDaly!

@StefanKarpinski StefanKarpinski added the feature Indicates new feature / enhancement requests label Sep 24, 2018
@StefanKarpinski StefanKarpinski added this to the 1.1 milestone Sep 24, 2018
@NHDaly NHDaly deleted the splitpath branch September 26, 2018 14:06
@NHDaly
Copy link
Member Author

NHDaly commented Sep 26, 2018

Hooray! :D Thanks for the merge!

And thanks for your help, and the back-and-forth! It was really fun essentially pair-programming on this! 😋
Cheers!

@StefanKarpinski
Copy link
Member

Would you be willing to make a PR adding a NEWS entry as well?

NHDaly added a commit to NHDaly/julia that referenced this pull request Oct 1, 2018
@NHDaly
Copy link
Member Author

NHDaly commented Oct 1, 2018

Would you be willing to make a PR adding a NEWS entry as well?

Ah, yeah, lemme take a stab at that!
#29448

NHDaly added a commit to NHDaly/julia that referenced this pull request Oct 1, 2018
@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Oct 3, 2018

  1. I see in the tests: # Unix-style paths are understood by all systems.

I didn't really see from the code how it passes those tests, but anyway, it that's really wanted (I guess so if they work on Windows, not just in Julia), do mixed paths also work, e.g. C:\path/to\something

If they do Julia code should also work with that (not sure it does currently).

  1. I'm not sure it's wanted, maybe it's actively not wanted, but should splitpath, also normalize them?

@StefanKarpinski
Copy link
Member

You could always try it...

julia> Windows.splitpath("C:\\path/to\\something")
4-element Array{String,1}:
 "C:\\"
 "path"
 "to"
 "something"

The normalization question doesn't make sense if you think about for a bit.

@NHDaly
Copy link
Member Author

NHDaly commented Oct 3, 2018

Haha to be fair, i didn't know about the cool Windows.<function> trick until you told me either!

But yep, they do, and also there are a few tests for it here:
https://github.com/JuliaLang/julia/pull/28156/files#diff-7629aea6e21fcaba2e27cc0a32eee2abR118

@StefanKarpinski
Copy link
Member

(The Windows trick only works if you define the module as I did above.)

NHDaly added a commit to NHDaly/julia that referenced this pull request Oct 4, 2018
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
fredrikekre added a commit that referenced this pull request Nov 30, 2018
fredrikekre added a commit that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 1, 2018
fredrikekre added a commit that referenced this pull request Dec 3, 2018
fredrikekre added a commit that referenced this pull request Dec 4, 2018
fredrikekre added a commit that referenced this pull request Dec 4, 2018
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.

Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
fredrikekre added a commit that referenced this pull request Dec 5, 2018
Addition of NEWS and compat admonitions for important changes between Julia 1.0 and 1.1, including:

- Custom .css-style for compat admonitions.

- Information about compat annotations to CONTRIBUTING.md.

- NEWS.md entry for PRs #30090, #30035, #30022, #29978,
  #29969, #29858, #29845, #29754, #29638, #29636, #29615,
  #29600, #29506, #29469, #29316, #29259, #29178, #29153,
  #29033, #28902, #28761, #28745, #28708, #28696, #29997,
  #28790, #29092, #29108, #29782

- Compat annotation for PRs #30090, #30013, #29978,
  #29890, #29858, #29827, #29754, #29679, #29636, #29623,
  #29600, #29440, #29316, #29259, #29178, #29157, #29153,
  #29033, #28902, #28878, #28761, #28708, #28156, #29733,
  #29670, #29997, #28790, #29092, #29108, #29782, #25278

- Documentation for broadcasting CartesianIndices (#30230).
- Documentation for Base.julia_cmd().
- Documentation for colon constructor of CartesianIndices (#29440).
- Documentation for ^(::Matrix, ::Number) and ^(::Number, ::Matrix).

- Run NEWS-update.jl.


Co-authored-by: Morten Piibeleht <morten.piibeleht@gmail.com>
Co-authored-by: Fredrik Ekre <ekrefredrik@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests filesystem Underlying file system and functions that use it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants