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

Respect .editorconfig settings for formatting shell via shfmt #2031

Merged
merged 15 commits into from
Feb 12, 2024

Conversation

tcrawford-figure
Copy link
Contributor

@tcrawford-figure tcrawford-figure commented Jan 30, 2024

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

@@ -23,7 +23,7 @@
import com.diffplug.spotless.shell.ShfmtStep;

public class ShellExtension extends FormatExtension {
private static final String SHELL_FILE_EXTENSION = "*.sh";
private static final String SHELL_FILE_EXTENSION = "**/*.sh";
Copy link
Contributor Author

@tcrawford-figure tcrawford-figure Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tcrawford-figure tcrawford-figure changed the title Add .editorconfig support for shfmt shell formatting Respect .editorconfig settings for formatting shell via shfmt Jan 31, 2024
@tcrawford-figure tcrawford-figure marked this pull request as ready for review January 31, 2024 01:50
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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@tcrawford-figure tcrawford-figure Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tcrawford-figure tcrawford-figure Feb 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but now might be the best time to do it

That would be great!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"}",
"repositories { mavenCentral() }",
"spotless {",
" shell {",
" shfmt()",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
plugin-gradle/README.md Outdated Show resolved Hide resolved

[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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 47 to 48
void shfmtMultipleFilesWithEditorconfig() throws IOException {
String fileDir = "shell/shfmt/multifile/with-config/";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! I can merge those tests together.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

plugin-gradle/README.md Show resolved Hide resolved
@@ -23,28 +23,32 @@ jobs:
uses: actions/checkout@v4
with:
fetch-depth: 0

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Goooler Goooler requested a review from nedtwigg February 8, 2024 13:52
@nedtwigg
Copy link
Member

Amazing teamwork, great PR!

@nedtwigg nedtwigg merged commit f5bfb34 into diffplug:main Feb 12, 2024
14 checks passed
@tcrawford-figure tcrawford-figure deleted the issue-2014/shfmt-editorconfig branch February 13, 2024 13:27
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 this pull request may close these issues.

shfmt - Shell support should respect .editorconfig configurations instead of using defaults
3 participants