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: support editorconfig properties for -s and -mn #819

Open
jfly opened this issue Feb 27, 2022 · 3 comments · May be fixed by #1062
Open

cmd/shfmt: support editorconfig properties for -s and -mn #819

jfly opened this issue Feb 27, 2022 · 3 comments · May be fixed by #1062

Comments

@jfly
Copy link

jfly commented Feb 27, 2022

Right now I have to keep my text editor's autoformatting code in sync with my project's Makefile to invoke shfmt. It would be nice to keep that version controlled on my project's .editorconfig file. Is that possible? If not, is it something you'd be open to a PR adding support for?

Thanks!

jfly added a commit to jfly/asdf-direnv that referenced this issue Feb 27, 2022
I spent some time this weekend getting my neovim configuration polished
up for working on shell scripts.

Once I got my editor auto-running `shfmt` for me, I found some
differences started to arise (for example, it was de-indenting all our
switch statements). Turns out we invoke `shfmt` with some non-default
settings (such as `-ci`), and my editor didn't know about that. I poked
around a little bit hoping to find some custom `.shfmtrc` file or
something we could put in the root of our repo. Turns out shfmt actually
knows about `.editorconfig` files, which we already have in this repo!

From
https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#description:

> If any EditorConfig files are found, they will be used to apply
> formatting options. If any parser or printer flags are given to the
> tool, no EditorConfig files will be used.

We were passing `-i 2` and `-ci`, which are both "printer flags", shfmt
was ignoring our `.editorconfig` file.

The fix is as simple as removing these two flags and updating our
`.editorconfig` file to have corresponding rules:

- We already had `indent_size = 2` in our `.editorconfig`. Bonus: we
  don't have to specify our indent size in 2 places anymore!
- I added `switch_case_indent = true` to our `.editorconfig` file. I
  guess this is a shfmt specific extension to editorconfig? It's
  documented here:
  https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples,
  but I don't see any mention of it here:
  https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties
  At first, I scoped this specifically to `.bash` files, but then I
  realized we've written shell code in files with other extensions (such as
  `bin/install` and `test/use_asdf.bats`). So, I've left this rule
  applicable to all files. It doesn't seem to cause any weirdness when I
  edit a file where it makes no sense (such as our `Makefile`).

Unfortunately, it doesn't look like there's a way of specifying `-s` via
the `.editorconfig` file, so I've had to leave that in our `Makefile`
for now. I've filed mvdan/sh#819 upstream with
the `shfmt` folks asking about this.
jfly added a commit to jfly/asdf-direnv that referenced this issue Feb 27, 2022
I spent some time this weekend getting my neovim configuration polished
up for working on shell scripts.

Once I got my editor auto-running `shfmt` for me, I found some
differences started to arise (for example, it was de-indenting all our
switch statements). Turns out we invoke `shfmt` with some non-default
settings (such as `-ci`), and my editor didn't know about that. I poked
around a little bit hoping to find some custom `.shfmtrc` file or
something we could put in the root of our repo. Turns out shfmt actually
knows about `.editorconfig` files, which we already have in this repo!

From
https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#description:

> If any EditorConfig files are found, they will be used to apply
> formatting options. If any parser or printer flags are given to the
> tool, no EditorConfig files will be used.

We were passing `-i 2` and `-ci`, which are both "printer flags", shfmt
was ignoring our `.editorconfig` file.

The fix is as simple as removing these two flags and updating our
`.editorconfig` file to have corresponding rules:

- We already had `indent_size = 2` in our `.editorconfig`. Bonus: we
  don't have to specify our indent size in 2 places anymore!
- I added `switch_case_indent = true` to our `.editorconfig` file. I
  guess this is a shfmt specific extension to editorconfig? It's
  documented here:
  https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples,
  but I don't see any mention of it here:
  https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties
  At first, I scoped this specifically to `.bash` files, but then I
  realized we've written shell code in files with other extensions (such as
  `bin/install` and `test/use_asdf.bats`). So, I've left this rule
  applicable to all files. It doesn't seem to cause any weirdness when I
  edit a file where it makes no sense (such as our `Makefile`).

Unfortunately, it doesn't look like there's a way of specifying `-s` via
the `.editorconfig` file, so I've had to leave that in our `Makefile`
for now. I've filed mvdan/sh#819 upstream with
the `shfmt` folks asking about this.
@mvdan
Copy link
Owner

mvdan commented Feb 28, 2022

Hmm. If you look at shfmt -h, you will notice that flags like -s and -mn aren't grouped with the parser or printer options, because they are neither of those - instead, they are extra steps which are run between the parse and the print stages.

I hadn't really thought about whether these two should be supported in editorconfig files. I guess if one squints a bit, they could be considered printer flags, as they alter how a shell script is printed out.

I can't really think of a reason why we shouldn't add support for them, so let's do it. I lean towards doing both rather than just -s, just for the sake of consistency.

@mvdan mvdan changed the title Is there an editorconfig flag for -s (simplify the code)? cmd/shfmt: support editorconfig properties for -s and -mn Feb 28, 2022
amrox pushed a commit to amrox/asdf-direnv that referenced this issue Apr 3, 2022
I spent some time this weekend getting my neovim configuration polished
up for working on shell scripts.

Once I got my editor auto-running `shfmt` for me, I found some
differences started to arise (for example, it was de-indenting all our
switch statements). Turns out we invoke `shfmt` with some non-default
settings (such as `-ci`), and my editor didn't know about that. I poked
around a little bit hoping to find some custom `.shfmtrc` file or
something we could put in the root of our repo. Turns out shfmt actually
knows about `.editorconfig` files, which we already have in this repo!

From
https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#description:

> If any EditorConfig files are found, they will be used to apply
> formatting options. If any parser or printer flags are given to the
> tool, no EditorConfig files will be used.

We were passing `-i 2` and `-ci`, which are both "printer flags", shfmt
was ignoring our `.editorconfig` file.

The fix is as simple as removing these two flags and updating our
`.editorconfig` file to have corresponding rules:

- We already had `indent_size = 2` in our `.editorconfig`. Bonus: we
  don't have to specify our indent size in 2 places anymore!
- I added `switch_case_indent = true` to our `.editorconfig` file. I
  guess this is a shfmt specific extension to editorconfig? It's
  documented here:
  https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples,
  but I don't see any mention of it here:
  https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties
  At first, I scoped this specifically to `.bash` files, but then I
  realized we've written shell code in files with other extensions (such as
  `bin/install` and `test/use_asdf.bats`). So, I've left this rule
  applicable to all files. It doesn't seem to cause any weirdness when I
  edit a file where it makes no sense (such as our `Makefile`).

Unfortunately, it doesn't look like there's a way of specifying `-s` via
the `.editorconfig` file, so I've had to leave that in our `Makefile`
for now. I've filed mvdan/sh#819 upstream with
the `shfmt` folks asking about this.
@sellout
Copy link

sellout commented Jan 2, 2023

While they’re separate flags, there’s really a progression of none → simplify → minify, so I would recommend a single editorconfig option accepting those three values as an enumeration. Not sure what a good name for it is, though. Perhaps something like rewrite_style?

@mvdan
Copy link
Owner

mvdan commented Jan 2, 2023

You're right that it doesn't make sense to use minify without simplify, and I don't even think it is currently possible. But I do want the editorconfig knobs to mirror the flags one-to-one, as otherwise it's a bit confusing. If the two flags being separate is weird, we should unify them, probably for v4.

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.

3 participants