Closed mvdan closed 4 years ago
An alternative approach is to make -ln=auto
do the same as -ln=bash
when formatting stdin or any files directly (without directory walking). This would keep the simple uses of shfmt
the way they are, and a bit simpler, but it would make the overall tool more inconsistent.
I don't think you will ever find a project which mixes posix/bash scripts together. If there is even a single bash script, chances are all scripts, even if they set /bin/sh, are only tested in bash and will not work with other shells anyway (due to small incompatibilities, unintended use of extensions or simply forgot to set /bin/bash).
Libraries (which are included with source
) never include shebang and parsing them differently to other files will lead to inconsistencies.
The big advantage is that this will flag shell scripts using #!/bin/sh and bashisms at the same time, which is a bug.
It should be possible to do without implementing auto
.
TLDR: choice of a shell is a project wide decision and implementing this flag will bring more harm than good.
I disagree that projects stick to one shell. At work we have repos that mix bash and posix. Bash for development scripts that are simpler to write with bashisms, and posix for scripts that need to run inside deployed containers which only include busybox.
are only tested in bash ... or simply forgot to set /bin/bash
In that case I think they should be forced to change the shebang to bash
, and I think that's a good thing. They might be mildly annoyed, but shfmt v3.0 will bring other small incompatible change that might annoy them anyway.
never include shebang and parsing them differently to other files will lead to inconsistencies.
They'll default to bash, which will parse pretty much any shell script out there; I'm not sure that this would be a problem in practice. I get your point about the inconsistency, but if they want explicit consistency, they can use shebangs or -ln=<whatever>
for the entire project.
It should be possible to do without implementing
auto
.
I'm not convinced; see my example above about our work repos. How would we catch those? Using shfmt -ln=posix
for the entire repo won't work, as some scripts are Bash. And we can't make the default be posix instead of bash, because that's just not the world we live in unfortunately.
The only alternative for us to catch those errors is to manually implement this shell language detection logic ourselves, or to use a separate tool to "detect incorrect bashisms". Both seem like worse options to me.
Another option is to add a new "lint" tool in this repo that would catch this error. But of course, practically noone would run this tool, and existing shfmt
users wouldn't benefit at all.
Actually shfmt -w .
doesn't format files without shebangs at all (for example APKBUILD
in alpine)
Another option is to add a new "lint" tool
This actually makes a lot of sense, especially if you port some good checks from https://github.com/koalaman/shellcheck
I think shfmt shouldn't have lang
flag at all and make it a job for a linter.
shfmt
does need a language flag, because the parser has the option. Parsing everything as Bash wouldn't be enough, because some of Bash's features are backwards-incompatible with POSIX.
I am aware of shellcheck, but it implements a lot of checks, and I'm just too lazy to implement something as complex as that :)
It just occurred to me that this has a huge overlap with #393. At the very least, I shouldn't add -ln=auto
until I've figured out language support as part of editorconfig files. So I'm closing this for now.
I've resurrected this idea in https://github.com/mvdan/sh/issues/622, by the way.
This way, when one does
shfmt -l -w .
, files with#!/bin/sh
shebangs would be formatted as if-ln posix
had been specified. The tool already has to parse shebangs when detecting if extension-less files are actually shell scripts, so this wouldn't be extra overhead.The logic for
-ln=auto
is as follows:bash
The big advantage is that this will flag shell scripts using
#!/bin/sh
and bashisms at the same time, which is a bug. A*.sh
script without a shebang that also uses bashisms won't be caught, but I think doing that would be far too prone to false positives; calling such a script likebash foo.sh
is common, and not all that bad. The logic would ignore filename extensions altogether.On the flip side, the
-ln
flag will become a bit more complex. I think I can explain the new logic with an extra line or two in the usage text.Thoughts?