-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
cmd/shfmt: Files with non-shell extension excluded #944
Comments
Interesting edge case. I did it this way for two reasons:
I think your case is a bit special because |
I don't think a valid Go file can have a valid shebang at the beginning, can it? And your shebang regex checks for the shebang at the beginning of a file, so this shouldn't be a problem.
Yeah, I see that point. There are situations where performance isn't that relevant, like in CI, where it would be okay to run a minute instead of a second. You could add a flag, but I understand that isn't a good solution either. |
Right, in practice it's not really a Go file. But I would argue the file doesn't look like a shell file, either. The Go toolchain and IDEs would pick it up as a Go file, for example. That's what I mean by being conservative - we should likely not format files which don't appear to be shell files. After all, it's always possible to format extra files if you need to (e.g.
I'm not opposed to a new feature (like a new flag) as long as it's reasonable and commonly needed - I think this scenario is a bit too niche to warrant a flag. If I were you, assuming that these shell files with odd extensions are easy to find (either via |
Another option in terms of new flags would be to turn |
FWIW, my IDE (Emacs) does recognize a file named It seems, strictly speaking, that if a file has any executable bit set, then only the shebang should be checked, and if it has no executable bit set, then only the extension should be checked (since no shebang will have an effect). This matches my own usage, where I use |
I actually thought about using executable bits; regardless what a file's extension is, if it has an executable bit set, we could look for a shebang inside it. Since executable files are relatively rare in software development scenarios, it could be a nice trade-off conceptually. However, it would slow down formatting directory trees, especially when there are many files. We use For more information, see golang/go#42027. I'm still happy to entertain a flag to tell shfmt to spend extra CPU time statting, opening, and reading many more files than it usually would. But I definitely don't want to make it the default behavior, because it will be much slower when there are many non-shell files, which is common. Walking one directory with a thousand Go files would suddenly go from one syscall (readdir) to over a thousand (readdir + 1000*lstat), for example. |
Yeah, agreed on the default. Maybe a flag like |
All changes in this commit were automated using the command: shfmt -w -i 2 -ci -bn . $(find . -name "*.sh.in") By default, only *.sh and files without extension are checked, so *.sh.in files have to be added additionally. (See mvdan/sh#944)
All changes in this commit were automated using the command: shfmt -w -i 2 -ci -bn . $(find . -name "*.sh.in") By default, only *.sh and files without extension are checked, so *.sh.in files have to be added additionally. (See mvdan/sh#944) (manually replayed commit 4cb8b13)
All changes in this commit were automated using the command: shfmt -w -i 2 -ci -bn bin/tests/system/ util/ $(find bin/tests/system/ -name "*.sh.in") By default, only *.sh and files without extension are checked, so *.sh.in files have to be added additionally. (See mvdan/sh#944) (manually replayed commit 4cb8b13)
Hi, I'm using
shfmt -f .
to list files I want to format. However, many files aren't found as they don't have a .sh/.bash extension, but are named something likemkosi.finalize
and I can't change the name as it is expected by some consumer. The files have, however, a valid shebang.The files are excluded because of the following line:
sh/fileutil/file.go
Line 80 in 279b806
Would be great to just omit it, and do the shebang check in any case.
The text was updated successfully, but these errors were encountered: