Remove types_or from shellcheck hook

[futtetennista]

This made the pre-commit configuration work with shell files. Otherwise it works only with .bash files. I couldn't really figure out why that's the case by looking at how pre-commit filtering works.

Tested on macOS 10.15.7 (Intel i7) and 12.6 (Apple M1 Max) using both bash and zsh.

[domenkozar]

cc @roberth

[roberth]

Not all shells are supported by shellcheck.
#63

I think we're going to need some tests for shellcheck.
Just merged

• testers: Add testBuildFailure and testEqualContents NixOS/nixpkgs#196251

so we can grep our build failures easily

[futtetennista]

I see. I guess one option would be to have different configurations based on the architecture? Or do you have any other idea?

[NeverBehave]

For anyone who read here. The current situation is if you have a .sh file, it won't be checked by shellcheck since pre-commit won't give exact match of what kind of script it is and whether it is supported by shellcheck.

• https://github.com/pre-commit/identify#with-a-file-on-disk
• Non-executable shell script with shebang line should be detected as "shell" pre-commit/identify#350

If you have a file ends with some known extension (e.g. some-bash.sh), it will not read the file content and skip shebang checks. Thus it won't have bash label inside. The reason is reading files is slow.

Another thing to notice is that executable attr will be treated differently. If your script is not executable it won't be checked if it does not have proper extension: https://github.com/pre-commit/identify/blob/cdad607f6a10bda406f7836568edbfd74eb4c6af/identify/identify.py#L62

All of these are really really unexpected, and I don't think this make the fact that file will be missing tags clear enough. For now my solution will be just create a custom hook to fit my project needs, as all of my script are based on bash.

However I do agree that make it configurable for now would be great, or adding some note in the hook.nix as a reference.

[roberth]

I think most projects do the sensible thing and

• have a shebang in their executables
• have a .sh extension in their non-executable files (or .bash)

We should support these and perhaps not worry too much about other shells. Perhaps we could move the current solution behind a settings flag?

[NeverBehave]

Right now the most annoying problem would be any .sh file, no matter what shebang it has, will be skipped, and I don't think this is expected behavior. This is a common convention for existing projects, even though it might now be a good one. But changing all them needs to put lots of effort

Related reading about .sh convention: https://stackoverflow.com/a/27824204

[roberth]

.sh file, no matter what shebang it has [...] Related reading

I agree with the linked explanation, and I should have added that I intended my bullet points to be mutually exclusive. To summarize

• executable:
  • has shebang,
  • has executable permission,
  • should not be loaded with . or source statements unless you take the effort to make the file dual-purpose, which I wouldn't recommend unless you can only distribute a single file,
  • no extension.
• non-executable:
  • no shebang
  • no executable permission,
  • is only loaded with . or source statements,
  • has an extension, .sh or .bash if you want to be honest about your bashisms (although personally I would keep it short with just .sh).

pre-commit's identify works with those rules, which is good, but unfortunately we don't get a good tag to grab onto for the case where the type of shell is unknown. Ideally it'd add a sh or unknown-shell tag, but as of now all we get is

>>> identify.tags_from_path("something.sh")
{'text', 'non-executable', 'shell', 'file'}

So for now we could just match shell and accept that we might in very rare cases try to check an unsupported shell file as well. Ideally we'd add sh or unknown-shell to identify, so that we check known-supported files, as well as unidentifiable files, but not known-unsupported files. (I guess this is confusing to read. The right pairing of words is: "known by identify", "supported by shellcheck")

[NeverBehave]

@roberth I believe either of your approach is feasible --- imo identity should allow further exploration of shell type when extension matched (continue to check shebang on shell type), as shell type file are typically not too many and reading them should be feasible in most cases but an option to control the behavior would be great. However this is basically pushing the problem to upstream.

For now I think we agree that current hook filter is too strict and make it less useful in most situations (.sh executable file was skipped). My simple approach to address this would be have a setting to switch between strict rule(current one) and relaxed rule(match any shell), and default to strict one so existing behavior won't break, but user may easily switch in between. This imo would cover most of the real world situation.

[mmlb]

oops I did not see this when I opened up #339. Now that its been merged though this can be closed right?

[NeverBehave]

@mmlb I believe so, thanks for the PR!

[domenkozar]

this is fixed on master