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 #13455 ; joinPath(a,b) now honors trailing slashes in b (or a if b = "") #13467

Merged
merged 3 commits into from
Feb 26, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Feb 22, 2020

joinPath(a,b) now honors trailing slashes in b (or a if b == "")
in the sense that it ends with a trailing slash if b ends with a trailing slash, or if b is empty and a ends in a trailing slash.

see detailed test cases in PR.

changed behaviors since #13462 :

  ## changed behaviors
  doAssert joinPath("foo", "bar") == "foo/bar" # same as before
  doAssert joinPath("foo", "bar/") == "foo/bar/" # was: foo/bar
  doAssert joinPath("foo", "./") == "foo/" # was: foo
  doAssert joinPath("foo", "", "bar/") == "foo/bar/" # was: foo/bar
  doAssert joinPath("./", ".", "/.") == "." # was: ./ (bug with variadic joinPath)
  doAssert joinPath("", "lib/") == "lib/" # was: lib
  doAssert joinPath("foo", "") == "foo" # was: foo/

it's 100% consistent with how nodejs does it:

Welcome to Node.js v13.8.0.
Type ".help" for more information.
> path.join('foo', '')
'foo'
> path.join('foo', './')
'foo/'
> path.join('foo', '','bar/')
'foo/bar/'
> path.join('./','.','/.')
'.'
> path.join('usr','')
'usr'
> path.join('','lib/')
'lib/'

it's 100% consistent with how matlab, octave does it:

octave:2> fullfile('foo','bar')
ans = foo/bar
octave:3> fullfile('foo','bar/')
ans = foo/bar/
octave:4> fullfile('foo','')
ans = foo
octave:5> fullfile('foo/','')
ans = foo/
octave:6> fullfile('foo/','/bar')
ans = foo/bar

it's also consistent with how D does it

(as far as honoring trailing slash)
(but D differs in some other aspect, buildPath("foo", "/bar") would return "/bar" ; that's a separate topic)

rdmd --eval 'writeln(buildPath("baz/", ""))'
baz/
rdmd --eval 'writeln(buildPath("baz", ""))'
baz
rdmd --eval 'writeln(buildPath("baz", "baz"))'
baz/baz
rdmd --eval 'writeln(buildPath("baz", "baz/"))'
baz/baz/

python

it's now consistent with how python honors trailing slash on tail:
os.path.join("foo","bar")
'foo/bar'
os.path.join("foo","bar/")
'foo/bar/'
but now differs from python in behavior when tail is empty:
os.path.join("foo","")
'foo/'

note

the only controversial aspect in this PR is the edge case of whether
joinPath("foo", "") should be "foo" (as in nodejs, D, go, matlab, and this PR) or "foo/" (as in python).
IMO the way I did it makes more sense, with the semantic of "" being an invalid path (unlike "."), it collapses out, so that
joinPath(foo1, "", foo2, "", "", bar, "") reduces to joinPath(foo1, foo2, bar) for example; it's also more consistent with joinPath("", "") being ""

@timotheecour timotheecour force-pushed the pr_fix_13455_joinPath_trailing branch 2 times, most recently from 27ae092 to 18e342f Compare February 22, 2020 12:02
@Araq
Copy link
Member

Araq commented Feb 22, 2020

On Windows tests/stdlib/tos.nim fails with this PR. AFAICT.

@timotheecour
Copy link
Member Author

PTAL, fixed

@timotheecour timotheecour changed the title [pending #13462] fix #13455 ; joinPath(a,b) now honors trailing slashes in b (or a if b = "") fix #13455 ; joinPath(a,b) now honors trailing slashes in b (or a if b = "") Feb 23, 2020
@Araq Araq merged commit 0c312ad into nim-lang:devel Feb 26, 2020
@timotheecour timotheecour deleted the pr_fix_13455_joinPath_trailing branch February 26, 2020 10:41
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("", "") is "/" ; should be ""
2 participants