Closed haltcase closed 6 years ago
super cool! wow, lots of updates, thanks!
[x] @citycide you should use ospaths.FileSystemCaseSensitive (it's buggy because of https://github.com/nim-lang/Nim/issues/8349 but I can send a PR to Nim soon) instead of re-defining the logic in IgnoreCase
(note: you did the same bug as in https://github.com/nim-lang/Nim/issues/8349, lol!)
[x] what's the rationale for https://github.com/citycide/glob/pull/19/commits/2f8d1a26d38a06d4cfbb00172616cbc7b84e5f03 feat: flip
ExpandDirsoption to
NoExpandDirs` ? see https://derickbailey.com/2014/03/27/double-negatives-in-code-dont-not-avoid-double-negative-boolean-logic/ it's easy enough to use
let options = defaultGlobOptions - {ExpandDirs}`
[x] nit: IgnoreCase => CaseInsensitive ? to match other terminology in makeCaseInsensitive
; or even better, use the name CaseSensitive in same spirit of avoiding double negation
[ ] wouldn't it make more sense / be simpler to add a flag CaseSensitive (defaulting to true) in https://github.com/citycide/glob/blob/master/src/glob/regexer.nim instead of transforming pattern in makeCaseInsensitive ?
[x] does FollowLinks take care of avoiding infinite recursion? wasn't clear from top-level msg
[ ] TODO(@timotheecour): compare example in #17 with unix find
[x] is UTF8 supported ? (eg in case of case sensitivity + other); whatever answer is, would be nice to specify in docs
[x] mention UTF8 limitations in docs
[ ] use ospaths.FileSystemCaseSensitive once https://github.com/nim-lang/Nim/issues/8349 is fixed
you should use ospaths.FileSystemCaseSensitive
I did use that for part of it, maybe it can be used more but I'd wait for that bug to get fixed.
what's the rationale for
`2f8d1a2 feat: flip
ExpandDirsoption
toNoExpandDirs``` ?
I noticed when putting together examples that it was easier to have fewer defaults because it makes it fairly simple when completely replacing them. I know it's pretty easy to just modify the defaults but this seems like a good balance. Generally I'd agree with you and wouldn't use a negated option but the ergonomics on this felt better.
nit: IgnoreCase => CaseInsensitive ? to match existing terminology in makeCaseInsensitive
That's an internal proc, shouldn't need to worry much about consistency between it and a public flag.
wouldn't it make more sense to add a flag CaseSensitive (defaulting to true) in https://github.com/citycide/glob/blob/master/src/glob/regexer.nim instead of transforming pattern in makeCaseInsensitive ?
That's what I did for the parser:
makeCaseInsensitive
is there because there's no way on case sensitive systems (that I know of) to list files case insensitively. So we turn each character in the string into a character class of its lower and upper case forms, then pass that as a pattern to Nim's os.walkPattern
to gather up all the matches.
does FollowLinks take care of avoiding infinite recursion? wasn't clear from top-level msg
Yep, see here:
is UTF8 supported ? (eg in case of case sensitivity + other); whatever answer is, would be nice to specify in docs
Not currently, we can leave it as a follow on and add it to the docs for now. It might work outside case sensitivity though.
@citycide please see https://github.com/citycide/glob/pull/20 ; seems much simpler than https://github.com/citycide/glob/commit/7115d8d39d420beaf4bf4c3f167105183576c305 for handling case insensitivity
I'm going to merge & release 0.8.0
soon unless anyone has something that should get into this first, but we can always follow up afterward.
Closes #8 (ref), closes #10 (ref), closes #11, closes #13 (ref), closes #17 (ref), closes #12 (ref)
API changes
Options (like
includeDirs
orincludeHidden
) are no longer provided as individual boolean parameters and are instead part of aset[GlobOption]
, the default for which is exported asdefaultGlobOptions
. You can alter this like you would any other set using unions and intersections:listGlob
removalThe
seq
-returning proclistGlob
is also removed to encourage the use of the iteratorswalkGlob
orwalkGlobKinds
. It's also easy enough to get aseq
from these usingsequtils.toSeq
from Nim's stdlib that it was mostly unnecessary, and this reduces the number of signatures we need to keep in sync.New features
Option flags
The new
GlobOption
enum is defined as:Several of these are equivalent to parameters in the current API, but a few of them are new:
relative = false
GlobOption.Absolue
expandDirs = false
GlobOption.NoExpandDirs
<dir>/**/*
includeHidden = true
GlobOption.Hidden
includeDirs = true
GlobOption.Directories
GlobOption.IgnoreCase
GlobOption.Files
GlobOption.DirLinks
GlobOption.FileLinks
GlobOption.FollowLinks
So this new options set allows for more granular things like only returning links:
More advanced filtering
When the options flags aren't enough, you can do a lot more with the optional
filterYield
orfilterDescend
procs (subsuming #10) to decide what gets yielded and what directories are traversed.Both of these receive a path as their first parameter (relative or absolute, depending on if
Absolute in options
).filterDescend
is called whenever the iterator comes across a directory that is about to be recursed into. If you return false, the iterator will abandon that recursion. Any path received by afilterDescend
proc is guaranteed to be a directory or a link to a directory.filterYield
is called whenever an item (file, directory, or link) is about to be yielded. If you return false, the iterator will not yield the item.These procs are called after options flags are checked, so if
Files notin options
then no paths pointing to files will be passed tofilterYield
.