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

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

Closed
wants to merge 2 commits into from

Conversation

stormsweeper
Copy link

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:

  • Written a clear PR title and description (above)
  • 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

Copy link
Collaborator

@benjaminjkraft benjaminjkraft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

generate/parse.go Outdated Show resolved Hide resolved
@csilvers
Copy link
Member

csilvers commented Apr 2, 2024

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
Copy link
Member

csilvers commented Apr 2, 2024

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

@benjaminjkraft
Copy link
Collaborator

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

benjaminjkraft commented Apr 5, 2024

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

@stormsweeper
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants