relativePath("foo", "foo") is now ".", not ""

[timotheecour]

as discussed many times already [1], empty dir shouldn't be considered as a valid dir and not conflated with ".", so relativePath("foo", "foo") is now "." and relativePath("", "foo") stays "".

[1] see #13236 (comment), #10018 (comment) + other places

• It is consistent with other things (eg parentDir("foo") is ".", not "")
• consistent with python:

os.path.relpath("foo","foo")
'.'

[Araq]

Not sure I'm buying it. ;-) But anyhow, same story as always:

• Needs a changelog entry.
• Needs to emulate the old behaviour with --useVersion:1.0.

[timotheecour]

Not sure I'm buying it. ;-)

it's the same story as in all previous cases where the behavior was corrected (see #13236 (comment), #10018 (comment) and more ) ... here's an example:

before PR:

  # this works, listing files under pwd/changelogs/
  for kind, path in walkDir(relativePath(getCurrentDir()/"changelogs", getCurrentDir())):
    echo (path,)

  # this returned nothing, instead of listing files under `pwd/changelogs/..` = `pwd`
  for kind, path in walkDir(relativePath(getCurrentDir()/"changelogs", getCurrentDir()/"changelogs")):
    echo (path,)

after PR:
works, no weird edge cases

Needs a changelog entry.

done

Needs to emulate the old behaviour with --useVersion:1.0.

I've instead added a -d:nimOldRelativePathBehavior and mentioned it in changelog as well as did a mention of it in of "useversion": section; these compatibility flags shouldn't be all or nothing as it makes it hard to "fix 1 problem at a time"; at least now user can selectively target what old behavior they need; although I doubt this old behavior would be needed. Also, defined(nimOldFooBehavior) scales well and is easy to search.

[Araq]

this returned nothing, instead of listing files under pwd/changelogs/.. = pwd
for kind, path in walkDir(relativePath(getCurrentDir()/"changelogs", getCurrentDir()/"changelogs")):
echo (path,)

Well tbh I find the behaviour that it doesn't iterate over anything more intuitive. x relative to itself is not the "current directory", that's pure guesswork. How do you know that x is the current dir? The current directory is what getCurrentDir returns, nothing else. Change your snippet to

for kind, path in walkDir(relativePath("doesntexist/changelogs", "doesntexist/changelogs")):
echo (path,)

to see it makes no sense.

works, no weird edge cases

For me it's "more weird edge cases".

[Araq]

I've instead added a -d:nimOldRelativePathBehavior and mentioned it in changelog as well as did a mention of it in of "useversion": section; these compatibility flags shouldn't be all or nothing as it makes it hard to "fix 1 problem at a time"; at least now user can selectively target what old behavior they need; although I doubt this old behavior would be needed. Also, defined(nimOldFooBehavior) scales well and is easy to search.

1. We want a single switch, --useVersion to emulate the version as good as reasonable.
2. nimOldX can be done in addition to that but it also doesn't "scale" because with N switches we get a test matrix of 2^N. Can't fight basic math.

[timotheecour]

done, --useVersion now emulates old behavior, simply by defining nimOldRelativePathBehavior
I get the idea of --useVersion for testing etc; but we also need nimOldX for making migrations easy by allowing users to fix one issue at a time. Obviously combinatorial testing is unpractical, so IMO the design --useVersion which itself defines -d:nimOldX1 -d:nimOldX2 etc gets us the best of both worlds: testable, and fine-tunable for users who need 1-at-a-time migration path

Change your snippet to
for kind, path in walkDir(relativePath("doesntexist/changelogs", "doesntexist/changelogs")):
to see it makes no sense.

it does make sense: change that snippet to for kind, path in walkDir(relativePath("doesntexist/changelogs", "doesntexist")):

it resolves to: for kind, path in walkDir("changelogs"): and lists the files under changelogs regardless of whether doesntexist exists or not. So it's entirely consistent.

x relative to itself is not the "current directory", that's pure guesswork

I'm not assuming that, in this context; relative directories are rebasable for relative path manipulations via the usual joinPath etc (which also happens to have a bug see #13455 joinPath("", "") is "/" ; should be "" ; but that's a separate issue)

note

this is consistent with go, D, python, and probably more

D:
rdmd --eval 'writeln(relativePath("/baz", "/baz"))'
.

package main
import ("fmt"; "path/filepath")
func main() {
  ex, err := filepath.Rel("/foo", "/foo")
  if err != nil { panic(err) }
  fmt.Println(ex)
}
.

python: see snippet in top post

[Araq]

it resolves to: for kind, path in walkDir("changelogs"): and lists the files under changelogs regardless of whether doesntexist exists or not. So it's entirely consistent.

It's terrible but I have to agree.

[timotheecour]

PTAL

[Araq]

I've thought about this a bit and alternatively we could have changed walkDir and friends so that "" means getCurrentDir. This way the observable behaviour of these procs wouldn't have changed so much and arguably "" is the same as "." so we fix yet another Windows/Posix bug by patching walkDir. Yay.

[timotheecour]

I've thought about this a bit and alternatively we could have changed walkDir and friends so that "" means getCurrentDir.

that would break many things

• here's one that you care about:

#13236 (comment)

What I do care about is that this loop just works:

with your proposal it would not work:

  import os
  var dir = getCurrentCompilerExe().parentDir # /pathto/Nim/bin
  dir = dir.relativePath(getCurrentDir())
  while dir.len > 0:
    for ai in walkDir(dir):
      echo ("files", dir, ai)
    dir = dir.parentDir()

under your proposal,

cd /pathto/
nim c -r main.nim # ok: would list all files under /pathto/Nim/bin/* /pathto/Nim/* /pathto/* 

cd /pathto/Nim
nim c -r main.nim # ok: would list all files under /pathto/Nim/bin/* /pathto/Nim/*

cd /pathto/Nim/bin
nim c -r main.nim # bug: should list all files under /pathto/Nim/bin/*
but instead would list not files at all because the loop `while dir.len > 0:` isn't even entered once in this case

• this would also break this case:

  import os
  var dir = getCurrentCompilerExe().parentDir
  dir = dir.relativePath(getCurrentDir())
  let root = "/usr/local"
  let dir2 = dir / root
  echo (dir2, isAbsolute(dir2))

cd /pathto/Nim/
nim c -r main.nim
("bin/usr/local", false) # ok

cd /pathto/Nim/bin
("/usr/local", true) # bug: should instead be ("./usr/local", false) or ("usr/local", false)

or you'd also have to change joinPath("", "/abspath") to be "./abspath"

• this would break semantics of RelativeDir, AbsoluteDir, un-initialized string as paths, etc.
  right now:

var path1: string
var path2: RelativeDir # (or AbsoluteDir)
path1.len == 0 has up to now been used to distinguish valid (ie, initialized) paths from invalid path (eg: un-initialized) paths.
if `path1.len == 0` were to have same semantics as `"."`, this would break those assumptions, creating lots of inconsistencies / code breakages

• There are other things this proposal would break.

this PR is consistent with D, go, python, C++, which do the right thing here:

rdmd --eval 'writeln(relativePath("/baz", "/baz"))'
.

package main
import ("fmt"; "path/filepath")
func main() {
ex, err := filepath.Rel("/foo", "/foo")
if err != nil { panic(err) }
fmt.Println(ex)
}

.

python3 -c 'import os; print(os.path.relpath("/foo","/foo"))'
.

[EDIT]

and also C++ for which I just out the relativePath closest equivalent : see https://en.cppreference.com/w/cpp/filesystem/path/lexically_normal
assert(fs::path("a/b/c").lexically_relative("a/b/c") == ".");

[Araq]

with your proposal it would not work

That's a pretty contrived example though. Not buying it, but I merged your PR already, so the discussion is rather pointless.