haltcase / glob

Pure Nim library for matching file paths against Unix style glob patterns.
https://glob.bolingen.me
MIT License
61 stars 5 forks source link

if pattern is absolute, don't transform it to relative by default (cf unix find) #23

Closed timotheecour closed 6 years ago

timotheecour commented 6 years ago

cd /tmp/foo

import glob

proc test()=
  let src_D="/tmp/foo/tests/nim/t48_glob/**/*" #NOTE: the pattern is absolute
  for a in walkGlobKinds(src_D): echo a

test()

prints:

(path: "tests/nim/t48_glob/test.nim", kind: pcFile)
(path: "tests/nim/t48_glob/a/b3.txt", kind: pcFile)

should print:

(path: "/tmp/foo/tests/nim/t48_glob/test.nim", kind: pcFile)
(path: "/tmp/foo/tests/nim/t48_glob/a/b3.txt", kind: pcFile)

EDIT so basically what I'm suggesting is to either change meaning of GlobOption.Absolute to have its default match behavior of unix find:

Absolute = true: all paths converted to absolute

Absolute = false and root = "" (the default) if pattern in relative, stay relative if pattern in absolute, stay absolute

Absolute = false and root != "" all paths converted to be relative to root (using relativePath from https://github.com/nim-lang/Nim/pull/8166#issuecomment-405685942 once ready; until then using whatever logic you have, which IIRC is currently different, as you don't allow ".." IIRC)

/cc @citycide what do you think?

haltcase commented 6 years ago

Isn't that confusing? Shouldn't providing GlobOption.Absolute just always determine how the paths are returned? I think I'd be surprised if I didn't pass Absolute but got back absolute paths. Think I could be swayed and it's important to match expectations from other implementations, just overall not sure.

I see why it might not make sense to receive relative paths when your pattern's absolute, but IMO that's what the flag is for.

timotheecour commented 6 years ago

the problem is current setup doesn't allow you to preserve absolute-ness of input pattern: if Absolute were a tri-state (off/on/auto) that would solve that issue. But since it's a 2-state bit, we need something else like checking root for empty; the way I suggest allows user to preserve absolute-ness by default.

It not only is more flexible but is what is standard pretty much everywhere:

haltcase commented 6 years ago

Alright I can be on board with that. So to summarize, here's how it should work:

Absolute in options ? root pattern is absolute? paths returned as
true irrelevant irrelevant absolute
false "" (default) no relative
false "" (default) yes absolute
false not "" irrelevant relative