Allow ** wildcards in globs. #5284
Conversation
quasicomputational
force-pushed
the
globstar
branch
from
8a00e00
to
7210b68
Compare
| match exactly, so ``*.gz`` matches ``foo.gz`` but not | ||
| ``foo.tar.gz``. A wildcard that does not match any files is an | ||
| error. | ||
| ``images`` directory. ``data-files: audio/**/*.mp3`` matches all |
There was a problem hiding this comment.
the documentation should mention that ** is available starting with cabal-version:3.0 (if we assume that 3.0 will be the next major release)
There was a problem hiding this comment.
Done. Also went ahead and changed all the '2.4' mentions to '3.0'.
|
I only gave this a cursory look, and so far this PR looks well made to me |
|
Looks like a very well crafted PR, kudos. What's the reasoning for the "must be the final directory bit"? (I'm stating all of the following as my personal opinion on the matter. I have no real decision-making power on this.) Generally, I'd object to making cabal's glob behavior even more distinct from the usual shell glob behavior. Yes, even if it is well documented as in this case. Most people don't actually read documentation and instead just copy examples and extrapolate from what the already know -- and then either give up or file issues when things don't work as they expect. Now, that is on them, but I predict that non-shell-like behavior (which even mimics the syntax of the shell!) will lead to more "support burden" for the cabal development team when issues/questions arise. I'd like to propose an alternative solution which I don't believe was discussed before: How about just using regex matches against the 'normalized' project-relative path? I.e. the whole string must match) against the path (using '/' as separator for cross-platformness). |
|
Looks nice on first glance, do you plan to implement or-patterns for file extensions as well ( |
|
@23Skidoo btw, you mentioning |
|
Not sure actually. |
It's forced by the data structure in the comment on #2522 by @23Skidoo, and it does slightly simplify implementation. This PR is targetting the absolute minimum possible version of this feature, to try and head off the bikeshedding that killed the previous one; after all, it's easier to lift restrictions in later versions than to add more. I'd rather see a limited patch merged and then iterated on rather than a more ambitious patch not get merged.
Not in this PR? I think globstar is worth having on its own merits, and that or-patterns can be separately implemented. They'd need new syntax and stealing
Agreed, but I think that's also separable work. A behaviour change to extension matching has different considerations (e.g., would
Nope, no way today; I asked a few months ago in #4935. At least this PR doesn't make things worse, since |
|
Sure, implementing those additional features I mentioned is not required for this PR to be accepted. |
|
Oops, this is buggy now. I know what I broke! |
|
OK, I fixed the bug I introduced, and also another target of opportunity I spotted while I was staring at SrcDist.hs. If Travis and the reviewers are happy, I think this is good to go (but I thought that before, too). |
quasicomputational
force-pushed
the
globstar
branch
from
bcf1661
to
cb36882
Compare
|
Note that we definitely need the fix from #5289 (otherwise we will do the wrong thing with |
quasicomputational
force-pushed
the
globstar
branch
2 times, most recently
from
811fbca
to
351903e
Compare
|
Fingers crossed I've fixed the Windows problem. I squashed and rebased, and this ought to be ready for review (again) if Travis and AppVeyor like it. |
These are inspired by a plan described in a comment in haskell#2522, and only implement a quite limited form of recursive matching: only a single ** wildcard is accepted, it must be the final directory, and, if a ** wildcard is present, the file name must include a wildcard. Or-patterns are not implemented, for simplicity. Closes haskell#3178, haskell#2030.
quasicomputational
force-pushed
the
globstar
branch
from
faaca90
to
6930a8d
Compare
|
I expanded the changelog note a bit, squashed and rebased again. CI ought to come back totally green since the persistent failures have been taken care of in master (hurrah!). If I don't get any objections, I'll merge this next week? |
|
|
||
| The reason for providing only a very limited form of wildcard is to | ||
| concisely express the common case of a large number of related files | ||
| of the same file type without making it too easy to accidentally | ||
| include unwanted files. | ||
|
|
||
| On efficiency: the directory tree will be walked starting with the |
There was a problem hiding this comment.
We might even want to add a warning about this.
| PatStem dir pat' -> | ||
| dir == seg && fileGlobMatchesSegments pat' segs | ||
| PatMatch Recursive ext -> | ||
| ext == takeExtensions (foldl' (flip const) seg segs) |
There was a problem hiding this comment.
(foldl' (flip const) seg segs)
IMO last (seg : segs) is more readable. Or we can vendor lastDef from the safe package.
|
Merged, thanks! |
PR haskell#5284 changed things around, and now matchDirFileGlob will break if it's passed a null directory, which happens to be the default value for data-dir. Its call sites have been fixed to check for this and to substitute '.' for an empty path, which is the desired behaviour; in addition, matchDirFileGlob itself will now warn about this if it's detected, so that new broken call sites can't sneak in. Fixes haskell#5318.
PR haskell#5284 changed things around, and now matchDirFileGlob will break if it's passed a null directory, which happens to be the default value for data-dir. Its call sites have been fixed to check for this and to substitute '.' for an empty path, which is the desired behaviour; in addition, matchDirFileGlob itself will now warn about this if it's detected, so that new broken call sites can't sneak in. Fixes haskell#5318.
PR haskell#5284 changed things around, and now matchDirFileGlob will break if it's passed a null directory, which happens to be the default value for data-dir. Its call sites have been fixed to check for this and to substitute '.' for an empty path, which is the desired behaviour; in addition, matchDirFileGlob itself will now warn about this if it's detected, so that new broken call sites can't sneak in. Fixes haskell#5318.
PR haskell#5284 changed things around, and now matchDirFileGlob will break if it's passed a null directory, which happens to be the default value for data-dir. Its call sites have been fixed to check for this and to substitute '.' for an empty path, which is the desired behaviour; in addition, matchDirFileGlob itself will now warn about this if it's detected, so that new broken call sites can't sneak in. Fixes haskell#5318.
PR #5284 changed things around, and now matchDirFileGlob will break if it's passed a null directory, which happens to be the default value for data-dir. Its call sites have been fixed to check for this and to substitute '.' for an empty path, which is the desired behaviour; in addition, matchDirFileGlob itself will now warn about this if it's detected, so that new broken call sites can't sneak in. Fixes #5318.
I fixed this in passing with haskell#5284, but let's add a test to make sure it stays fixed. The error message isn't perfect, but it's a lot better than failing silently! Closes haskell#5195.
This was only a convenience function, but its use could obscure how it is introducing a dependency on the CWD. By removing it, the "." argument to `matchDirFileGlob` is explicit. Any external code using `matchFileGlob` would have needed to be changed as haskell#5284 changed its signature and the module it lives in; it is not much more of a burden to switch to `matchDirFileGlob` at the same time.
This was only a convenience function, but its use could obscure how it is introducing a dependency on the CWD. By removing it, the "." argument to `matchDirFileGlob` is explicit. Any external code using `matchFileGlob` would have needed to be changed as haskell#5284 changed its signature and the module it lives in; it is not much more of a burden to switch to `matchDirFileGlob` at the same time.
This was only a convenience function, but its use could obscure how it is introducing a dependency on the CWD. By removing it, the "." argument to `matchDirFileGlob` is explicit. Any external code using `matchFileGlob` would have needed to be changed as haskell#5284 changed its signature and the module it lives in; it is not much more of a burden to switch to `matchDirFileGlob` at the same time.
This was only a convenience function, but its use could obscure how it is introducing a dependency on the CWD. By removing it, the "." argument to `matchDirFileGlob` is explicit. Any external code using `matchFileGlob` would have needed to be changed as haskell#5284 changed its signature and the module it lives in; it is not much more of a burden to switch to `matchDirFileGlob` at the same time.
This was only a convenience function, but its use could obscure how it is introducing a dependency on the CWD. By removing it, the "." argument to `matchDirFileGlob` is explicit. Any external code using `matchFileGlob` would have needed to be changed as #5284 changed its signature and the module it lives in; it is not much more of a burden to switch to `matchDirFileGlob` at the same time.
|
What are good defaults for Some packages depend on this: |
|
The You can find ready-made |
These are inspired by a plan described in a comment in #2522, and only
implement a quite limited form of recursive matching: only a single **
wildcard is accepted, it must be the final directory, and, if a **
wildcard is present, the file name must include a wildcard.
Or-patterns are not implemented, for simplicity.
Closes #3178, #2030.
[ci skip]is used to avoid triggering the build bots.Includes tests!