Ignored files not ignored when passed by name

[dcd-arnold]

Hi @mvdan,

thank you for shfmt. I have read that a directive in the .editorconfig like this:

# Ignore the entire "third_party" directory.
[third_party/**]
ignore = true

... should ignore the directory. However, this seems only true for the usage of --find. If I call shfmt third_party/script.sh, shfmt formats the file. I expect shfmt to output the file without any alternation, since the file is exempt from formatting.

Is this a bug or intended? I am asking because I am building a setup for CI and multiple editors and I would like to exclude some files from formatting.

Have a nice weekend,
dcd-arnold

[mvdan]

That is indeed expected. The "ignore" logic only applies when walking directories, like shfmt -w .. When giving filenames directly, the tool assumes that you do want to format that file no matter what.

In a way, why else would you be running that command? Or, put another way, what could the tool reasonably do? Failing, or printing no stdout rather than the formatted source, both seem counter-intuitive.

That said, this is not well documented - I will update the man page.

[dcd-arnold]

Hi @mvdan,

thank you for getting back to me that quickly.

That is indeed expected. The "ignore" logic only applies when walking directories, like shfmt -w ..

You do mean -f, right? Or am I not getting something?

When giving filenames directly, the tool assumes that you do want to format that file no matter what. In a way, why else would you be running that command?

I get the rational behind it. However, this places the burden to determine which file to call shfmt for on the caller. I have multiple use cases to address, e.g. CI, VSCode, neovim, and that on multiple repositories. Each caller needs some logic to determine which file to run on. Usually I stuff that in some config file and have the tool to "just work", agnostic to the caller.

Or, put another way, what could the tool reasonably do? Failing, or printing no stdout rather than the formatted source, both seem counter-intuitive.

I completely agree, printing no stdout and failing are both counter intuitive, and not very useful for that matter. I would like to propose to print the file without alternation, like cat. That way shfmt behaves very predictable (there is always the processed file on stdout). Which is also the way prettier behaves (I only checked now to make sure).

That said, this is not well documented - I will update the man page.

That would be very appreciated. However, the current behavior is rather counter-intuitive.

[mvdan]

You do mean -f, right? Or am I not getting something?

shfmt recursively walks directories when given a directory. This happens with shfmt -w ., or shfmt -f ., or whatever other flag.

However, this places the burden to determine which file to call shfmt for on the caller. I have multiple use cases to address, e.g. CI, VSCode, neovim, and that on multiple repositories. Each caller needs some logic to determine which file to run on. Usually I stuff that in some config file and have the tool to "just work", agnostic to the caller.

Unless I'm missing something, you could and probably should be running shfmt on each repository's directory. That's how the majority of people use the tool.

If what you're using is an editor or IDE which works on a file-by-file basis rather than the entire repository, and wants to integrate with shfmt, the easiest solution is likely for the integration to check for the ignore EditorConfig setting. An editor would already have to load that config anyway, I assume.

I would like to propose to print the file without alternation, like cat.

Sorry, but I disagree. If you're running a formatter on a filename explicitly, the behavior should not be to silently print it out as it already was.

[dcd-arnold]

You do mean -f, right? Or am I not getting something?

shfmt recursively walks directories when given a directory. This happens with shfmt -w ., or shfmt -f ., or whatever other flag.

I get it, I updated my suggestion to the PR, accordingly.

However, this places the burden to determine which file to call shfmt for on the caller. I have multiple use cases to address, e.g. CI, VSCode, neovim, and that on multiple repositories. Each caller needs some logic to determine which file to run on. Usually I stuff that in some config file and have the tool to "just work", agnostic to the caller.

Unless I'm missing something, you could and probably should be running shfmt on each repository's directory.

That is kind of my point. I am not calling shfmt by myself in every scenario. For editor integration, there are two VSCode extensions available. shfmt and shell-format, both act on on a by-file-basis.

That's how the majority of people use the tool.

How do you know?

If what you're using is an editor or IDE which works on a file-by-file basis rather than the entire repository, and wants to integrate with shfmt, the easiest solution is likely for the integration to check for the ignore EditorConfig setting. An editor would already have to load that config anyway, I assume.

Well, then the editors behavior would differ form your tool. Not exactly intuitive either.

I would like to propose to print the file without alternation, like cat.

Sorry, but I disagree. If you're running a formatter on a filename explicitly, the behavior should not be to silently print it out as it already was.

When I look for other formatters on github (there are only few with more stars than shfmt), I see prettier which excludes files even when called with a file as an argument. And I see black, which provides a --force-exclude for that case.

If you think your tool should be used differently, I completely respect that. However, I will have to come up with another solution for my use cases. Thank you anyway for a great tool and being so open for discussion.

[ciarand]

Hello! We ran into this problem using the rules_lint package (a set of Bazel-related linters and formatters), which calls shfmt FILENAME1 FILENAME2 FILENAME. Instead of providing a built-in ignore list, the rules_lint package defers to the formatters' native configuration files. As far as I can tell, shfmt's native configuration file is editorconfig (which is very cool btw).

Would you be open to a --force-exclude flag being added to shfmt? This would mirror the behavior of black's --force-exclude functionality (thanks @dcd-arnold for the research). I'd be happy to contribute an MR if you'd be willing to review it.

[alexeagle]

I didn't verify this, but I imagine users of pre-commit will have the same problem, since it runs tools on the changed files by passing their paths to the tool.

[mvdan]

Thanks all for your input. Trying to recap what is my understanding of the situation below.

I think there are two use cases for shfmt:

1. Users calling it directly, e.g. shfmt -l -w . to format all files, shfmt -w foo.sh to format one file, or e.g. print-shell | shfmt to format stdin.

2. Tools like IDEs calling shfmt to format files for the user; from my understanding, this is often shfmt -w foo.sh or some variant like shfmt -filename=foo.sh to capture stdout instead.

This seems to be mainly about use case 2, and I agree it's awkward. My earlier suggestion was that the tool should check editorconfig to see if a file should be ignored, but that's effectively reimplementing our logic, which isn't a good solution. I definitely want to make some change here, I'm just not sure which.

I would like to retain the property that, regardless of "ignore" settings, shfmt foo.sh should always format the file. Changing this could silently break some users who currently assume the formatting does happen. And there should be a way to force formatting of a file no matter what, so if we changed it, we would still need to a flag like --no-ignore. So I don't think this option (what prettier does) is good for us.

Thanks for bringing up psf/black#438. That seems to be an almost exact replica of our situation and my thinking as a maintainer :) I think adding a flag like --force-exclude for the use case 2 seems reasonable; I think that most people on use case 1 would simply ignore the flag, and that's fine. The only nit is that I might call the flag something like --apply-ignore to better mimic our EditorConfig setting.

Assuming such an --apply-ignore flag, here's how it might behave:

• shfmt --apply-ignore -w foo.sh would become a no-op when the file is ignored. This is easy, since -w can skip writing to the file, and similarly, -l and -d can skip printing anything at all.
• shfmt --apply-ignore foo.sh or print-shell | shfmt --apply-ignore -filename=foo.sh is tricky, since the current behavior when formatting without -w/-l/-d is to print the formatted shell to stdout unless we fail. I think we have three options when the file is ignored:
  • Print it back out unchanged. This would best match -w/-l/-d, but it is a bit odd that calling a formatter results in cat behavior.
  • Print absolutely nothing. Tools could identify this case to mean "nothing to do". I don't think this can conflict with any existing behavior, since we always format shell files with a trailing newline, even empty ones, e.g. shfmt <<<'' | wc -c. This might conflict with EditorConfig's insert_final_newline=false if we ever support it, but I don't think we ever will.
  • Fail with a known exit code or error string, to clearly signal to the tool that there's nothing to do - basically a clearer form of the previous option.

What does black --force-exclude do with this last choice? None of the three seem clearly best to me.

[dcd-arnold]

Thank you for getting back to us.

black does not write to stdout, it overwrites the file on disk.
However, if black is called on an excluded file, it gives a exit code of 0 and a message:

No Python files are present to be formatted. Nothing to do 😴

Simply running shfmt on a folder with no files to format results in exit code 0 and no output. That is what I would expect running shfmt on an ignored file. Hence, my vote goes to:

Print absolutely nothing.

[mvdan]

I also lean towards "print absolutely nothing". I initially had a minor worry that this could lead to IDE/editor integrations suddenly emptying people's shell scripts, but the integration would have to explicitly opt into this behavior via --apply-ignore, so at that point they should start handling the "empty stdout" case. Effectively, this flag exists only for programs like editors or scripts, which can handle the ignored case.

[mvdan]

Please take a look at this PR implementing the above: #1051

[dcd-arnold]

Please take a look at this PR implementing the above: #1051

@mvdan Thank you. I would like to share two observations:

1. When checking a file which is formatted correctly, an empty line is printed. It is not printed when the file is ignored. It would be more consistent if the output would match.

2. I would like to raise concern regarding the name of the setting within the editorconfig file. Maybe ignore = true is too generic and format = false would be more appropriate.

[mvdan]

1. When checking a file which is formatted correctly, an empty line is printed. It is not printed when the file is ignored. It would be more consistent if the output would match.

Sorry, can you clarify? I don't follow.

[mvdan]

2. I would like to raise concern regarding the name of the setting within the editorconfig file. Maybe ignore = true is too generic and format = false would be more appropriate.

Perhaps you're right. We made up this property name because upstream doesn't have a standard way to ignore/skip files. For better or worse, ignore = true has existed in shfmt for three years now (2c15805), so I'm reluctant to rename it and break users unless we really have to. At this point, I think it's fine to keep as-is and see if upstream decides to add a standard feature in the future, at which point we can entirely deprecate ours.

[dcd-arnold]

1. When checking a file which is formatted correctly, an empty line is printed. It is not printed when the file is ignored. It would be more consistent if the output would match.

Sorry, can you clarify? I don't follow.

Sorry, my test case must have been flawed. I am not able to reproduce a setup in which the statement 1. is valid. It works perfectly as far as I am concerned.

[mvdan]

Thanks for the input :) Merging and gearing up for a release.

[mvdan]

Release done; tools and editors should hopefully be able to start relying on this as users upgrade. They could either check if shfmt --apply-ignore with an empty stdin succeeds or not, or compare shfmt --version with the v3.8.0 semver.

[ciarand]

Thanks @mvdan!