Respect `.editorconfig` settings for formatting shell via `shfmt` by tbcrawford · Pull Request #2031 · diffplug/spotless

tbcrawford · 2024-01-30T22:16:13Z

Closes #2014. Note that this removes the previous "default" formatting settings for shfmt that was provided by spotless in the previous versions.

tbcrawford · 2024-01-30T22:24:19Z


 public class ShellExtension extends FormatExtension {
-	private static final String SHELL_FILE_EXTENSION = "*.sh";
+	private static final String SHELL_FILE_EXTENSION = "**/*.sh";


It's very likely that project's aren't storing their shell scripts as siblings to the build file so this updates the search to be more inclusive by default.

Goooler · 2024-02-01T01:54:35Z

 		String format(ProcessRunner runner, String input, File file) throws IOException, InterruptedException {
 			if (args == null) {
-				args = List.of(exe.confirmVersionAndGetAbsolutePath(), "-i", "2", "-ci");
+				args = List.of(exe.confirmVersionAndGetAbsolutePath(), file.getAbsolutePath());


What if the second run, still using the first file path?

Hm.. Looks like this formatter doesn't work when there is more than one file to format. I'll have to make sure I add a test case for multiple files that need formatted and verify the output. The second file ends up containing the formatted contents of the first file that was formatted. ~~Do you have any suggestions here?~~ I assumed that the file variable was going to contain the current file that needs formatted, but that doesn't seem to actually be the case.

I rescind my ask. I was able to format it properly via standard input.

@Goooler this should all be fixed now. The solution turned out to be simpler than I thought in the end. Please re-review this PR at your earliest convenience 🙂

From my observation, --filename is not a necessary param for shfmt, we can remove it. Am I missing anything here?

Based on the documentation your assumption/observation would be correct. However, without the --filename parameter and the path, the .editorconfig settings aren't respected.

I did some digging to try to understand this better myself. This commit for shfmt seems to explain why the filename flag is necessary when reading from stdin: mvdan/sh@fa7035e

And this was the original issue related to the change: mvdan/sh#577

If we must input the file path here, it might be something like Goooler@ae75240?

but now might be the best time to do it

That would be great!

If we must input the file path here, it might be something like Goooler@ae75240?

Would you be fine with something like the following instead? I don't think args needs to be an instance variable as of right now. If args need provided to the State constructor eventually, then someone can refactor it for their needs. Unless args is somehow used implicitly for something that I'm unaware of.

String format(ProcessRunner runner, String input, File file) throws IOException, InterruptedException { final List<String> args = List.of(exe.confirmVersionAndGetAbsolutePath(), "--filename", file.getPath()); return runner.exec(input.getBytes(StandardCharsets.UTF_8), args).assertExitZero(StandardCharsets.UTF_8); }

The only concern is if it's necessary to check the executable file for every formatting. Otherwise, I'm okay with your snippet.

I took some time to see what you're talking about here and how this all works. I will absolutely go with your original suggestion. It makes a lot more sense. I didn't realize that args would be reused once it was set the first time. This explains why I kept seeing the same filename in my testing too.

Goooler · 2024-02-01T01:56:05Z

+				"repositories { mavenCentral() }",
 				"spotless {",
 				"  shell {",
 				"    shfmt()",


If we can add an option to override .editorconfig path like BaseKotlinExtension.KtlintConfig#setEditorConfigPath?

From what I can tell, when configuring ktlint for formatting, you actually provide the .editorconfig path to ktlint and that is an input supported by ktlint. As far as I can tell, there is no way to provide an .editorconfig path to shfmt 🙁

With that said, I did request that an input flag be added for situations like this: mvdan/sh#1055

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

Goooler · 2024-02-02T08:49:11Z

+
+[homepage](https://github.com/mvdan/sh). [changelog](https://github.com/mvdan/sh/blob/master/CHANGELOG.md).
+
+When formatting shell scripts via `shfmt`, configure `shfmt` settings via `.editorconfig`.


If we can note a bit more here, that the .editorconfig file only be supported at the same level as shell files, you can't override it's path due to mvdan/sh#1055.

That is an incorrect assumption, surprisingly. If I were to have the following project structure, all shell files will respect the .editorconfig settings. I can write tests to validate and solidify this claim, but this is the behavior I've seen in a standalone test project. shfmt tries to be smart when looking for an .editorconfig. I'm not sure I know what the process for finding the .editorconfig is, but the developer's https://github.com/mvdan/editorconfig library is used instead of the default Go library and I think that plays a factor in finding the editorconfig.

repository-name/ ├── client/ │ └── scripts/ │ ├── foobar.sh │ └── barfoo.sh ├── scripts/ │ ├── foo.sh │ └── bar.sh ├── .editorconfig ├── init.sh └── build.gradle.kts

Goooler · 2024-02-02T08:55:06Z

+	void shfmtMultipleFilesWithEditorconfig() throws IOException {
+		String fileDir = "shell/shfmt/multifile/with-config/";


The only difference between shfmtMultipleFilesWithoutEditorconfig with shfmtMultipleFilesWithEditorconfig is the last folder in the path, can we merge them to using @ParameterizedTest?

We can only keep these 2 cases, and remove shfmtWithoutEditorconfig and shfmtWithEditorconfig, it should be enough.

Yeah! I can merge those tests together.

I ended up creating a more complex test directory to validate multiple shell scripts. In attempting to merge some of the tests, I realized that it would take some checking based on parameterized input to determine if there is more than 1 file to check and thought that looked a little ugly and maybe hard to understand.

I believe testing a complex directory structure should cover the necessary test cases, but please let me know if you really prefer having single file tests for some reason.

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

Goooler · 2024-02-06T02:02:55Z

        uses: actions/checkout@v4
        with:
          fetch-depth: 0
+


Please revert these format changes, including the array style changes.

nedtwigg · 2024-02-12T23:02:28Z

Amazing teamwork, great PR!

tbcrawford commented Jan 30, 2024

View reviewed changes

Comment thread lib/src/main/java/com/diffplug/spotless/shell/ShfmtStep.java Outdated

tbcrawford commented Jan 30, 2024

View reviewed changes

tbcrawford changed the title ~~Add .editorconfig support for shfmt shell formatting~~ Respect .editorconfig settings for formatting shell via shfmt Jan 31, 2024

tbcrawford marked this pull request as ready for review January 31, 2024 01:50

Goooler reviewed Feb 1, 2024

View reviewed changes

tbcrawford added 4 commits February 1, 2024 09:19

Stash progress

4898462

Add .editorconfig support for shfmt

d95436a

Revert unnecessary changes

4ae13c9

Update change logs and documentation for shell formatting

1b3335d

tbcrawford force-pushed the issue-2014/shfmt-editorconfig branch from c2ae79b to 1b3335d Compare February 1, 2024 14:19

Make files private

733f7da

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

tbcrawford mentioned this pull request Feb 1, 2024

cmd/shfmt: document how --filename relates to EditorConfig support mvdan/sh#1055

Closed

tbcrawford added 2 commits February 1, 2024 13:51

Fix formatting multiple shell files at the same time

3c2701d

Add multi-file shfmt tests for maven

8bf8d8f

tbcrawford requested a review from Goooler February 1, 2024 20:25

Goooler reviewed Feb 2, 2024

View reviewed changes

tbcrawford and others added 7 commits February 2, 2024 09:00

Fix plugin-gradle/README.md typo

4c4b7bc

Co-authored-by: Zongle Wang <wangzongler@gmail.com>

Fix how Shfmt args are constructed, tidy tests

2c38247

Include shfmt check in ci build

28b079c

Fix version regex to only match digits and '.'

62f27ca

Remove sudo, it should be unnecessary

42b43dc

Fix comment

fc82b24

Add shfmt man page link to gradle readme

06325f8

tbcrawford requested a review from Goooler February 5, 2024 22:31

Goooler approved these changes Feb 8, 2024

View reviewed changes

Undo yaml formatting changes, update spotless to Spotless

6e52d6e

Goooler requested a review from nedtwigg February 8, 2024 13:52

nedtwigg merged commit f5bfb34 into diffplug:main Feb 12, 2024

tbcrawford deleted the issue-2014/shfmt-editorconfig branch February 13, 2024 13:27


		[homepage](https://github.com/mvdan/sh). [changelog](https://github.com/mvdan/sh/blob/master/CHANGELOG.md).

		When formatting shell scripts via `shfmt`, configure `shfmt` settings via `.editorconfig`.

		void shfmtMultipleFilesWithEditorconfig() throws IOException {
		String fileDir = "shell/shfmt/multifile/with-config/";

Conversation

tbcrawford commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

tbcrawford Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbcrawford Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbcrawford Feb 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nedtwigg commented Feb 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tbcrawford commented Jan 30, 2024 •

edited

Loading

tbcrawford Jan 30, 2024 •

edited

Loading

tbcrawford Feb 1, 2024 •

edited

Loading

tbcrawford Feb 1, 2024 •

edited

Loading