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

fmt: warn if glob matches nothing #3918

Closed

Conversation

kevinkassimo
Copy link
Contributor

Closes #3915

$ ./target/debug/deno fmt fdsafdasf.ts
Warning: 'fdsafdasf.ts' does not match any files
Formatted 0 files in 817ns

@hayd
Copy link
Contributor

hayd commented Feb 8, 2020

In a script (which is the only time exit code really matters) running 0 tests is always going to be a bug and so IMO it should exit(1).

@kevinkassimo
Copy link
Contributor Author

@hayd I believe this might be debatable? We can add a flag to turn this warning into an error, or if more people think it is a good default, invert the flag to turn error (default) into warning

@hayd
Copy link
Contributor

hayd commented Feb 8, 2020

What's the counter-argument?

@nayeemrmn
Copy link
Collaborator

@hayd The warnings are applied per-glob here. It would be too strict to give a non-zero exit code for any glob that didn't match anything. That said, I would prefer if it matched the test runner.

Maybe alongside the per-glob warnings, if no provided glob matched anything then we can exit(1)?

@dplewis
Copy link

dplewis commented Feb 8, 2020

@hayd I agree with you on zero tests but the PR is about formatting files.

@ry
Copy link
Member

ry commented Feb 8, 2020

I’m not sure why we need to be expanding globs at all in Deno... Isn’t this taken care of by the shell?

@nayeemrmn
Copy link
Collaborator

@ry Things like default globs, directory expansion, a useful --exclude flag will require internal glob handling. This particular usage aside, having no good way to expand globs within cli is too crippling to get away with. I worry that #3865 will be more than a short-term regression when I hear justifications for not needing this capability.

@ry
Copy link
Member

ry commented Feb 13, 2020

Thanks Kevin, I was about to merge this, but as I was fixing the conflicts I noticed some other things and ended up doing a whole refactor.

#3988

@ry ry closed this Feb 13, 2020
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.

Deno fmt command does not complain about missing files
5 participants