Include e2e folder in shfmt editorconfig for 2 spaces indenting #1937
Conversation
Without this, shfmt has a different format for *.sh vs e2e/test_* files which are also shell scripts.
|
Strange. Maybe shfmt on CI is different than local or how we run shfmt isn't respecting the editorconfig. |
|
would it be simpler to use shfmt's default style maybe? |
.editorconfig
Outdated
| @@ -2,7 +2,7 @@ | |||
| indent_style = space | |||
| indent_size = 2 | |||
|
|
|||
| [{*.sh,.mise/tasks/**/*}] | |||
| [{*.sh,.mise/tasks/**/*,e2e/*}] | |||
There was a problem hiding this comment.
this isn't ideal since it's missing test_* files in subdirectories. I think this is another good argument for sticking with shfmt's native behavior so we don't need to define this in editorconfig necessarily
There was a problem hiding this comment.
Looks like it was a function of shellcheck and shfmt usage in 7285064.
They were constraining the files considered for formatting and how it will include subdirectories.
| @@ -2,7 +2,7 @@ | |||
| indent_style = space | |||
| indent_size = 2 | |||
|
|
|||
| [{*.sh,.mise/tasks/**/*}] | |||
| [{*.sh,.mise/tasks/**/*,e2e/**/*}] | |||
There was a problem hiding this comment.
This may still be a problem since I think this is globbing files which are not scripts. There are some other files in this directory. I'm not certain though
There was a problem hiding this comment.
The way I think it behaves is:
- editorconfig tells shfmt what to consider in scope aside from other instructions
- in mise run lint and lint-fix, it's explicitly stated what to format and fed to shfmt, so shfmt restricts itself to those files, which didn't include the subdirectories
I have a commit later to address it.
There was a problem hiding this comment.
one thing I seem to have to relearn every few years is that file extensions are pretty much always a good idea :)
We could but I've seen shfmt default be less common than Google's shell styleguide in the bash ecosystem. shfmt has an example of the Google shell style guide in their own documentation: https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd#examples and I prefer it because the behavior more closely matches what's commonly used in many other languages (2x spaces = an indent level). If editorconfig doesn't do it, explicitly putting the Google flags for the two places shfmt is run would ensure it's consistent in the codebase. from shfmt docs: I wanted to offer this change b/c it's currently an inconsistent mix of spaces and tabs. I think the uniformity is most important (tabs or spaces) followed by choosing which. Also, this is a minor nit :) that I wanted to offer for my own simplicity in the project. I'm ok w/ it being closed too as working in a way that's acceptable. |
|
🤔 I don't think CI is respecting .editorconfig. |
|
admittedly I have a weird mix of CI tools that might be conflicting a bit here—specifically megalinter and the |
|
@jdx That's what I just came back to fill in as a comment :)
|
|
oh looks like you got it all working? is this good to merge? |
|
@jdx Yes, I took another look through and it's ✅ ready to merge. Thanks for talking through it. |
Right now e2e differs from other *.sh formatting in that e2e is autoformatted w/ tabs rather than spaces.
It looks like an error and I'll see if this change to editorconfig correctly includes the e2e folder.
I segregated the bulk formatting changes to separate commit, but it's many lines of change. The other approach can be gradually unifying the formatting as individual new PRs come in or an area is refactored.