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

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

Closed
timotheecour opened this issue Jul 21, 2018 · 3 comments
Labels
breaking Breaking changes will occur as part of closing the issue. good first issue Probably good for a user's first contribution. help wanted Extra attention is needed type: feature Request for a new feature or enhancement.

Comments

@timotheecour
Copy link
Collaborator

timotheecour commented Jul 21, 2018

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 nim-lang/Nim#8166 (comment) 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?

@timotheecour timotheecour changed the title if pattern is absolute, don't transform it to relative if pattern is absolute, don't transform it to relative by default Jul 21, 2018
@timotheecour timotheecour changed the title if pattern is absolute, don't transform it to relative by default if pattern is absolute, don't transform it to relative by default (cf unix find) Jul 21, 2018
@haltcase haltcase added breaking Breaking changes will occur as part of closing the issue. discussion [RFC] Fixes, APIs, or changes need feedback. type: feature Request for a new feature or enhancement. labels Jul 22, 2018
@haltcase
Copy link
Owner

haltcase commented Jul 23, 2018

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
Copy link
Collaborator Author

timotheecour commented Jul 23, 2018

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:

  • same as unix find
  • same as python glob
  • same as nim walkPattern
  • same as D's dirEntries
    etc...

@haltcase
Copy link
Owner

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

@haltcase haltcase added help wanted Extra attention is needed good first issue Probably good for a user's first contribution. status: accepted Change is accepted and is open to community PRs. and removed discussion [RFC] Fixes, APIs, or changes need feedback. labels Jul 23, 2018
haltcase pushed a commit that referenced this issue Jul 24, 2018
BREAKING CHANGE: absolute patterns now result in absolute paths if no root is provided (even if `Absolute notin options`

Closes #23
@haltcase haltcase added status: in progress Work has started to close the issue. and removed status: accepted Change is accepted and is open to community PRs. labels Jul 27, 2018
haltcase added a commit that referenced this issue Aug 2, 2018
BREAKING CHANGE: absolute patterns now result in absolute paths if no root is provided (even if `Absolute notin options`)

Closes #23
@haltcase haltcase removed the status: in progress Work has started to close the issue. label Sep 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking changes will occur as part of closing the issue. good first issue Probably good for a user's first contribution. help wanted Extra attention is needed type: feature Request for a new feature or enhancement.
Projects
None yet
Development

No branches or pull requests

2 participants