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.

Sign up for GitHub

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

Jump to bottom

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

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

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

jfly opened this issue Feb 27, 2022 · 4 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
@jfly
Use EditorConfig for shftm "parser or printer flags"
0052a19 
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
@jfly
Use EditorConfig for shfmt "parser or printer flags"
2e56e15 
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 jfly mentioned this issue Feb 27, 2022
Merged
@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
@jfly @amrox
Use EditorConfig for shfmt "parser or printer flags"
6a95ea7 
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.

@mvdan mvdan mentioned this issue Feb 14, 2024
Closed
ColemanTom added a commit to ColemanTom/sh that referenced this issue Feb 23, 2024
@ColemanTom
cmd/shfmt: Add support for minify and simplify via EditorConfig
09202a7 
Closes mvdan#819.
@ColemanTom ColemanTom linked a pull request Feb 23, 2024 that will close this issue
Open
ColemanTom added a commit to ColemanTom/sh that referenced this issue May 23, 2024
@ColemanTom
cmd/shfmt: Add support for minify and simplify via EditorConfig
0acaa23 
Closes mvdan#819.
@ColemanTom
Copy link

I have raised a MR for this, keeping the names consistent with the command line options. If minifiy is enabled, simplify is automatically enabled too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

cmd/shfmt: Add support for minify and simplify via EditorConfig ColemanTom/sh
4 participants
@sellout @jfly @mvdan @ColemanTom