cmd/shfmt: add a way to skip custom directories when walking directories

[paulbellamy]

When recursing it would be nice to be able to ignore certain directories or files (for example, node_modules).

Either a --exclude=node_modules, or --ignore=node_modules, or a .shfmtignore-type functionality would be nice.

[mvdan]

I'd imagine that .shfmtignore wouldn't be a great long-term solution. If every tool had their own dot-ignore files, repositories would end up with dozens of them. I wonder if it would make sense to simply piggyback on .gitignore.

Otherwise, adding a -skip or -exclude flag sounds fine to me. The directory walking tool already has some hard-coded logic for that, so it would only be a matter of exposing it.

[mvdan]

Here's another question, Paul - is this about performance, or about not formatting files that shouldn't be formatted?

If it's the former, then we really do need a way to skip the massive directories.

If it's the latter, perhaps there's other workarounds. For example:

shfmt -f . | grep -v node_modules/ | xargs shfmt -l -w

[paulbellamy]

It's about being able to use shfmt as a linting tool, while not having the CI build fail if there are node_modules (containing shell scripts) present.

fwiw, I definitely agree about the .ignore files being clutter.

For now I'm using:

shfmt -d $(find . -type f -not -path './node_modules/*')

But I didn't see the -f flag (rtfm 😂), so will use that, thanks! Not a high priority issue, I guess.

[mvdan]

I added -f precisely because finding all the shell scripts in a repository is non-trivial, so I figured it would be useful for other tools and scripts. But I guess it does work for this usecase :)

Right now this is what we skip:

var vcsDir = regexp.MustCompile(`^\.(git|svn|hg)$`)
[...]
if info.IsDir() && vcsDir.MatchString(info.Name()) {
        return filepath.SkipDir
}

So this would likely translate to a default of -skip=.git,.svn,.hg. Then you could use a custom list, like -skip=.git,node_modules. It would also skip a .git file if there was one, but I think that's fine.

The other option is to not add the flag and instead tell people to use a combination of shfmt -f and tools like grep. I'd like to think about the two options for a few days - at the moment, I'm undecided.

[mvdan]

For now, I think users should stick to:

shfmt -d $(shfmt -f . | grep -v node_modules/)

This can be extended however one likes, and should be fast enough. If a directory like node_modules ever becomes truly massive, and walking it is too expensive, one could always skip it before calling shfmt, like:

shfmt -d $(shfmt -f $(find . -maxdepth 1 -type d -not -path './node_modules/'))

So I'm going to close this for now, as I don't see a big need to add another flag. If anyone thinks otherwise, please speak up or open another issue.

[lamyergeier]

Please reopen this issue as this solution doesn't work with pre-commit formatters like lintstaged. Also, .shfmtignore should be used as it is a standard way of ignoring files.

[mvdan]

Please reopen this issue as this solution doesn't work with pre-commit formatters like lintstaged.

That seems like a limitation of that piece of software. If a formatter tool is given a list of files, it should format those files.

In other words, if what you're saying was true, lots of other formatters like gofmt would need fixing too.

Also, this issue is about walking directories (like shfmt -w .) and not formatting specific files, which is what that pre-commit software seems to do. If you find a bug in how shfmt handles file arguments, please file a separate issue.

Also, .shfmtignore should be used as it is a standard way of ignoring files.

If every tool had config and ignore files on each repo using said tool, we'd end up with dozens of dot-files bothering developers. I'll only consider adding either if there really is a big need for it. Otherwise, just following what's standard for the sake of it is not a good idea.

[fpuga]

First thanks for this great software.

This comment is not for requesting anything just for adding another point of view.

I'm using the patterns that @paulbellamy and @mvdan showed in this issue to exclude or even something like shfmt -f . | grep -v -f .shfmtignore to simulate the existence of an ignore file. But as the grep expressions are not gitignore like expressions is a bit difficult to maintain the .shfmtignore file.

Some tools like stylelint allow a --ignore-path .ignore where .ignore is a file with gitignore like syntax that can be shared for different tools. And others like black are thinking in implementing it.

In my case i like that this ignore file takes precedence even over files passed from command line (in git hooks) because sometimes is useful to copy&paste an external file or set of files from third party that i prefer to keep as is.

[mvdan]

We're going to do #393, so shfmt will now have a (shared) per-project config file. I think that's a fine place to be able to configure something like this.

For example, there's editorconfig/editorconfig#228. It hasn't progressed as a new EditorConfig feature, but we can still grab ideas from there.

[soullivaneuh]

The .editorconfig way will not solve the issue. Extension file are generally specified and it does not manage file ignorance.

Managing a .shfmtignore file would be a great addition. It would be also great to specify a custom ignore file like this:

shfmt --ignore-path .gitignore

By the way, providing a --ignore-path would solve the interrogation you made in #288 (comment).

A lot of lint tools already provide this option like eslint, prettier or even stylelint.

I already implemented it on a project thanks to this library: github.com/sabhiram/go-git-ignore

You can see the implementation here: https://gitlab.com/nexylan/pretty/blob/acaa4fa25f5bddabb1b289c86dcad9f324c6193f/main.go#L114-133

As you can see, it is quite simple to implement ignore file and it will simplify a lot the usage of this tool. 👍

[soullivaneuh]

Please see my proposal: #422

[mvdan]

I left a short review on the PR. I generally disagree with the design; it adds a custom -if flag and a custom .shfmtignore file on disk.

I realise that editorconfigs don't directly support ignoring files, but that's worth a try over adding yet another config file and flag.

[soullivaneuh]

I really don't see the issue to add a flag or a new config file (if you specify .gitignore usage, you don't need to put an extra .shfmtignore file.) as all is optional.

The added logic is not complex and fix this issue with ease and feels everyone needs.

We are not discussing about adding or changing a rule nor adding an option about that, but adding a needed feature to globally improve the tool usage.

Plus, the fact many lint tools already implement the option should encourage us to follow the way IMHO. 👍

Maybe I'm missing something about your concern, and I would be glad to better understand.

[soullivaneuh]

For anybody, here is my workaround for .gitignore usage:

FILES=$(comm -12 <(shfmt -f . | sort) <(git ls-files | sort))
if [ -z "${FILES}" ]; then
	exit 0
fi
echo "${FILES}" | xargs shfmt ${OPTIONS}

But it it not ideal as we have a strict dependency to git here.

[mvdan]

Please just let me add editorconfig support first. One major new feature at a time :) The time I have to work on this project is limited, and new flags and features need to be carefully considered.

[kaey]

Amount of dot-files in big projects is already alarming, we should try hard not to add another one.
In gofmt usual approach is to build file list by some other tool and pass it to gofmt (https://github.com/golang/go/blob/release-branch.go1.12/misc/git/pre-commit)
Can you use the same approach in your projects?

I do not have any opinion on editorconfig.

[soullivaneuh]

Please just let me add editorconfig support first. One major new feature at a time :)

This is not the same thing and, honestly, my MR is not so big. And it's ready to use! :-)

Amount of dot-files in big projects is already alarming

This is why I let the option to use .gitignore directly. The amount of dot-files in big project is a matter of dev choice.

Can you use the same approach in your projects?

You meant, add a pre-commit? No, because it's for a CI tool.

Here is my implement (with git workaround): https://gitlab.com/nexylan/pretty/blob/master/tools/shfmt/run

But this is quite buggy: https://gitlab.com/nexylan/pretty/issues/44

BTW, if I do not find a more reliable workaround, I may have to remove shfmt support from the project.

@mvdan Having this tiny option I proposed in #422 would make thing a lot easier and this approach is already adopted by many popular let tools. Could you please tell me why you are still uncomfortable to it?

Regards

[kaey]

This is why I let the option to use .gitignore directly.

Adding more flags is just as bad as adding more dotfiles.

The amount of dot-files in big project is a matter of dev choice.

It's not, when tool doesn't work well without that dotfile.

You meant, add a pre-commit? No, because it's for a CI tool.

No. I meant make a tool which builds list of files and feeds it to shfmt (this is what referenced pre-commit hook does).

[mvdan]

@soullivaneuh other tools doing something, or a patch being ready for a feature, doesn't mean that we must add that feature. As I said above, I carefully consider new features, particularly those that add new flags or encourage the use of yet more dotfiles.

I'm pretty happy with the EditorConfig support. I'd very much prefer to build on top of that instead of adding more config files. For example, to ignore all node_modules directories under the current dir:

[node_modules/**]
ignore = true

The shfmt tool would stop walking a directory sub-tree as soon as an ignore = true property was found. This should be as powerful as a file like .gitignore, because one can have nested EditorConfig files, and their glob matching is extremely similar to git's.

The only downside that I see is that, in practice, this could mean duplicating some lines between .gitignore and .editorconfig. However, I think that's acceptable, for a number of reasons:

• Rarely does a project have more than two or three huge directories with tons of files to ignore
• Following .gitignore would be unfair to other version control systems such as hg or svn
• Adding a flag to allow .shfmtignore bloats the tool and dotfiles in the long run

It's unfortunate that this ignore = true idea never caught on with the official EditorConfig spec, but I think it's still okay for us to define and use it as a custom property.

I'll ship v3 without this feature, but we can add it soon after in master for v3.1 if it sounds like a good plan to others. Particularly interested in @paulbellamy's input, though it's been over a year since he filed the issue :)

[mvdan]

I laid out my plan above two months ago, and I still think it's a good idea for the reasons stated in that last comment.

The implementation is done; I'll push it to close this issue. 3.1.0 will come sometime in March, so there are a few weeks of time if anyone has feedback on the feature. I'm going to close #422 for now, because it accomplishes something pretty similar but at the cost of adding one more flag and tool-specific config file.

[soullivaneuh]

It maybe be fill @paulbellamy issue, but not the .gitignore management one.

Our tool pretty manage lint tool execution regardless of the submitted project to allow us configuration tool reducing.

The .editorconfig is more a IDE configuration file that a ignore file. .gitignore is far more used.

So no, your changes does not bring anything on this side.

Could you please reopen #422 for discussion? The option implement is very tiny and this is the may most of the lint tools manage the ignore files.

It's really hard to me to understand your concerns. :-/

[soullivaneuh]

BTW, you also refused the .gitignore native management (without) option for an another solution also forcing the use of a specific file (.editorconfig).

With a ignore file option, we may not manage .editorconfig (both can also be implemented), but we let the user possibility to manage any versioning system ignore file they want.

Even if I personally use it, I don't see so many project using the .editorconfig file.

If your only concern is the option addition, could we at least detect and manage populare ignore files like you did for the .editorconfig?

Regards.

[mvdan]

The .editorconfig is more a IDE configuration file that a ignore file. .gitignore is far more used.

It's not just for editors. Build systems like Gradle and Maven also support it, for example, and I think it fits shfmt pretty nicely.

So no, your changes does not bring anything on this side.

Sorry, but I disagree - if anyone badly needs to make shfmt ignore a few subdirectories, I don't see why adding a .shfmtignore or a flag is OK but adding an .editorconfig is not.

The option implement is very tiny and this is the may most of the lint tools manage the ignore files.

The code being tiny isn't really a factor, though. The fact that other tools do something can count, but it doesn't mean that it's automatically a good idea for us.

If your only concern is the option addition, could we at least detect and manage populare ignore files like you did for the .editorconfig?

The flag was one concern, yes, but the other concern is config files. Most users needed a way to set flags via a file, and some needed to ignore directories via a file. A gitignore only fixes the second, smaller problem. An editorconfig fixes the bigger first problem, and with a bit of a stretch, it fixes the second too.

It's of course not a perfect solution, but it seems to me like a far better solution than supporting two sets of config files, when in fact we don't really need two config files.

It's really hard to me to understand your concerns. :-/

That's fine :) The joy of open source is that everyone gets to voice their opinion. But, as a maintainer, that also makes some decisions terribly difficult. How does one make a decision when it's impossible to make everyone in a thread happy? How does one reject a patch without discouraging the author from contributing again in the future? Honestly, maybe I'll make mistakes, but I'm only doing what I think is best for the project in the long run. I hope you can understand that.

[mvdan]

I also forgot to mention - thanks to @tbcs and issue #477, it should now be far easier to use shfmt in scripts that only want to reformat the subset of scripts which have been modified.

It also allows simplifying examples earlier in this thread; for example:

# format all shell scripts, except those in node_modules
shfmt -f . | grep -v node_modules/ | xargs shfmt -l -w

can probably be just the line below, assuming that node_modules is not part of the git repo:

# format all shell scripts contained in this git repo
shfmt -f $(git ls-files) | xargs shfmt -l -w

[msanders]

# format all shell scripts contained in this git repo
shfmt -f . | grep -v node_modules/ | xargs shfmt -l -w

For what it's worth, the examples in this thread are unsafe for filenames containing spaces (see here for more info). Technically they should be looped via something like:

shfmt -f . | grep -v node_modules/ | while read -r fname; do
    shfmt -d "$fname"
done

Although this still doesn't handle other special characters like newlines correctly.

Probably unlikely to come up with scripts in source repos but thought it was worth noting. It might make sense to add a -z or -0 flag for shfmt -f to use safely with xargs -0. (See e.g. find -print0, git ls-files -z, grep --null, etc.)

[kaey]

Although this still doesn't handle other special characters like newlines correctly.

shfmt -f output for filenames with newlines is completely broken, I suggest to file a new issue for that.

As for the fix - \0 separator can only work with xargs and xargs is bizzare. Working with filenames in shell is annoying, but I found that using proper shell escaping works okayish:

$ printf "%q\n" "long
name.sh" | sed 's/^/shfmt -d /' | cat
shfmt -d $'long\nname.sh'

Final cat can be replaced with bash to actually execute resulting script

printf is expand.Format in mvdan/sh terms.

while read syntax can be used as well with help of eval:

$ printf "%q\n" "long
name.sh" | while read -r x; do eval "shfmt -d $x"; done

None of that will work on windows of course and I don't know what to do with it.

[dtan2-wiq]

Thanks @mvdan for the great work in this library.

Just wanted to share how I configured shfmt to skip certain directories. Hopefully it'll save the next person a few hoops.

My problem: shfmt -w . was picking up issues in a dependencies folder (I had a Python project, so it was ./.venv, but it could well be node_modules for example)

My fix (as recommended in shfmt docs):

• Add .editorconfig file with the following:

[.venv/**]  # replace with the directory that you want shfmt to ignore
ignore = true

[sdavids]

I created #1095 because this issue has been addressed via the .editorconfig key ignore.

I do think that having --ignore in addition is worth having.