Only warn if a glob does not match any files, but error if total is 0

Khan / genqlient

a truly type-safe Go GraphQL client

MIT License

1.02k stars 99 forks source link

Only warn if a glob does not match any files, but error if total is 0 #328

Closed stormsweeper closed 3 months ago

stormsweeper commented 3 months ago

This changes the filename expansion to only warn on empty globs, and error only if no files in total were found.

This is a strawman proposal for issue #327

I have:

[x] Written a clear PR title and description (above)
[x] Signed the Khan Academy CLA
[?] Added tests covering my changes, if applicable No current tests exist, but I can add ones for this case and check error messages
[ x] Included a link to the issue fixed, if applicable
[ -] Included documentation, for new features
[ -] Added an entry to the changelog

csilvers commented 3 months ago

You make a good case! Arguably a better case for ** but this is easier. (And while reverting this after adding ** support would be a breaking change, it's probably an okay-ish one for v0.) One small comment below.

I agree ** would be a better solution to this problem, and nice for other reasons as well. How hard would it be to add it?

csilvers commented 3 months ago

(There's https://github.com/bmatcuk/doublestar or https://github.com/yargevad/filepathx we could use, if we wanted.)

benjaminjkraft commented 3 months ago

Probably not that hard, I think the only annoying thing is that different tools have subtle differences in how they interpret it so we need to make sure whatever we use documents their behavior clearly. (Off the top of my head, git interprets ** as "zero or more characters", whereas eslint does "zero or more full path components" so you have to do **/*.go or whatnot. It looks like doublestar you linked does the latter , and filepathx doesn't really specify.) And ideally make sure we use it everywhere we accept file-globs.

benjaminjkraft commented 3 months ago

Ok, sorry, Craig convinced me: let's do double-star globs (#330) instead. (I think that will solve your issue equally well, right?)

stormsweeper commented 3 months ago

@benjaminjkraft totally, happy to have spurred it along :)