Skip to content
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: skip zsh scripts when walking directories #535

Open
ferrarimarco opened this issue Apr 8, 2020 · 8 comments · May be fixed by #1101
Open

cmd/shfmt: skip zsh scripts when walking directories #535

ferrarimarco opened this issue Apr 8, 2020 · 8 comments · May be fixed by #1101

Comments

@ferrarimarco
Copy link

Hi, thanks for this tool! :)

I understand from #120 that zsh is not going to be supported, but the tool (tried with 3.1.0) still wants to format them. I use shfmt in my CI builds to check if the scripts are correctly formatted, and I suppose that if zsh is not supported, it shouldn't try to format it, right?

Example:

#!/usr/bin/env zsh

zstyle ':completion:*' list-colors "${(@s.:.)LS_COLORS}"

Output of shfmt -d .:

script.sh:3:39: parameter expansion requires a literal

This makes my build fail, unless I exclude zsh scripts and call shfmt -d on each file instead. So there's a workaround, but since shfmt does not support zsh, it shouldn't attempt to format them.

What do you think? Thanks!

@mvdan
Copy link
Owner

mvdan commented Apr 8, 2020

Thanks for the issue - I agree this is a bit weird. Right now, if a file has the right extension (in this case, .sh for POSIX Shell), then we don't bother to check the shebang line at all. I think in this case it would be reasonable to do so - we're going to be reading the file anyway, so it's no extra effort, and it can avoid some false positives like the one you found.

@mvdan mvdan added this to the 3.2.0 milestone Apr 8, 2020
@mvdan
Copy link
Owner

mvdan commented Apr 24, 2020

@texastoland see #120. It would be a lot of work to add support, and a lot of work afterwards to maintain (which would likely fall on just me). So the decision thus far is not to support it.

If someone wants to provide an implementation, and possibly maintain it in the long run, I'm all ears. But noone has stepped up so far :)

@mvdan mvdan changed the title Don't format zsh scripts cmd/shfmt: skip zsh scripts when walking directories May 25, 2020
@mvdan
Copy link
Owner

mvdan commented May 25, 2020

I've hit a bit of a roadblock here, and that's because of the -f flag to find shell scripts under a directory.

If we make shfmt -w . skip *.sh with #!/bin/zsh, that makes sense for formatting, but it would make -f inconsistent, as -f would still list those files.

If we make -f skip those files too, then arguably -f becomes less useful. The point of -f is to find all shell scripts, not just those that shfmt happens to support parsing.

The best alternative here is probably to support all common POSIX shell variants which use .sh extensions in the parser (including zsh), then we can keep both features aligned. Any language that's close enough to POSIX shell should be reasonable to add to the syntax package, and any language that's very different will not use the .sh extension, such as .fish.

@mvdan mvdan removed this from the 3.2.0 milestone Jul 18, 2020
@Mellbourn
Copy link

I think you should keep including zsh files. In my opinion, the current, somewhat broken support for zsh, is better than no support at all. shfmt still finds many meaningful issues in zsh code.

@mvdan
Copy link
Owner

mvdan commented Apr 5, 2021

It's increasingly likely that we will support zsh in the future, given it being a default for Mac, so I think that's the best long-term solution.

@thomasmerz
Copy link

@mvdan A long time has gone by sinvce your last comment… Is zsh already supported?

@mvdan
Copy link
Owner

mvdan commented Mar 25, 2022

@thomasmerz subscribe to #120 if you want to see the updates when there are any.

@mcandre
Copy link

mcandre commented Mar 22, 2023

It's increasingly likely that we will support zsh in the future, given it being a default for Mac, so I think that's the best long-term solution.

While we eagerly await zsh support in shfmt, it would fix more projects near term, to temporarily skip file paths of zsh files (*.zsh, *.zshrc, *.zshenv, #!/bin/zsh) in the short term.

As a workaround, I am using a custom sh family finding tool and telling it to skip zsh-related files, before passing them to shfmt:

stank -exInterp zsh . |
    grep -v node_modules |
    xargs shfmt [any other shfmt options here]

This is fragile (breaks on any file paths involving spaces). It requires grep, which may not necessarily be installed in all environments. It requires a separate process, which is wasteful. And it means having to manage a fairly long command in a build configuration file (Makefile, Rakefile, magefile.go, Shakefile.hs, vast.sh, CMakeLists.txt, rez.c/rez.cpp, etc.)

And if any files get accidentally copied to the local tree via go mod vendor, bundle install, or yet other package mangers, then the list of excluded directory patterns grows even longer.

Ideally, shfmt would automatically exclude more files which is cannot handle, and learn to integrate into industry standard tooling like .gitignore, regardless of other future plans for shfmt.

dcarley added a commit to dcarley/sh that referenced this issue Oct 9, 2024
To prevent `shfmt` from discovering `zsh` files which aren't currently
supported for formatting.

This fixes mvdan#535 and the following test:

    > cmpenv stdout find.golden
     diff stdout find.golden
     --- stdout
     +++ find.golden
     @@ -16,6 +16,4 @@
      modify/shebang-space
      modify/shebang-tabs
      modify/shebang-usr-sh
     -skip/ext.zsh
     -skip/shebang-zsh
      symlink/subdir/ext-shebang.sh

     FAIL: testdata/script/walk.txtar:10: stdout and find.golden differ

It could be considered a breaking change to `shfmt` and `fileutil`.

There's some discussion in that issue about whether it's beneficial to
match `zsh` files because they can _sometimes_ be formatted if they
don't contain any zsh-specific syntax, but without any way to explicitly
exclude them from discovery it's more of a hindrance than help.
@dcarley dcarley linked a pull request Oct 9, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants