Respect .editorconfig settings for formatting shell via shfmt

[tcrawford-figure]

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

[tcrawford-figure]

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]

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

[tcrawford-figure]

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.

[tcrawford-figure]

@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 🙂

[Goooler]

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

[tcrawford-figure]

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

[Goooler]

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

[Goooler]

but now might be the best time to do it

That would be great!

[tcrawford-figure]

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);
}

[Goooler]

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

[tcrawford-figure]

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]

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

[tcrawford-figure]

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

[Goooler]

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.

[tcrawford-figure]

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]

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

[Goooler]

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

[tcrawford-figure]

Yeah! I can merge those tests together.

[tcrawford-figure]

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.

[Goooler]

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

[nedtwigg]

Amazing teamwork, great PR!