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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ jobs:
- kind: npm
jre: 11
os: ubuntu-latest
- kind: shfmt
jre: 11
os: ubuntu-latest
shfmt-version: 3.7.0
runs-on: ${{ matrix.os }}
steps:
- name: Checkout
Expand All @@ -79,6 +83,14 @@ jobs:
- name: test npm
if: matrix.kind == 'npm'
run: ./gradlew testNpm
- name: Install shfmt
if: matrix.kind == 'shfmt'
run: |
curl -sSfL "https://github.com/mvdan/sh/releases/download/v${{ matrix.shfmt-version }}/shfmt_v${{ matrix.shfmt-version }}_linux_amd64" -o /usr/local/bin/shfmt
chmod +x /usr/local/bin/shfmt
- name: Test shfmt
if: matrix.kind == 'shfmt'
run: ./gradlew testShfmt
- name: junit result
uses: mikepenz/action-junit-report@v4
if: always() # always run even if the previous step fails
Expand Down
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
## [Unreleased]
### Added
* `FileSignature.Promised` and `JarState.Promised` to facilitate round-trip serialization for the Gradle configuration cache. ([#1945](https://github.com/diffplug/spotless/pull/1945))
* Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))

### Removed
* **BREAKING** Remove `JarState.getMavenCoordinate(String prefix)`. ([#1945](https://github.com/diffplug/spotless/pull/1945))
* **BREAKING** Replace `PipeStepPair` with `FenceStep`. ([#1954](https://github.com/diffplug/spotless/pull/1954))
Expand Down
16 changes: 13 additions & 3 deletions lib/src/main/java/com/diffplug/spotless/shell/ShfmtStep.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.List;
import java.util.Objects;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import javax.annotation.Nullable;

Expand Down Expand Up @@ -72,7 +74,7 @@ private State createState() throws IOException, InterruptedException {
"\n github issue to handle this better: https://github.com/diffplug/spotless/issues/673";
final ForeignExe exe = ForeignExe.nameAndVersion("shfmt", version)
.pathToExe(pathToExe)
.versionRegex(Pattern.compile("(\\S*)"))
.versionRegex(Pattern.compile("([\\d.]+)"))
.fixCantFind(howToInstall)
.fixWrongVersion(
"You can tell Spotless to use the version you already have with {@code shfmt('{versionFound}')}" +
Expand All @@ -83,9 +85,11 @@ private State createState() throws IOException, InterruptedException {
@SuppressFBWarnings("SE_TRANSIENT_FIELD_NOT_RESTORED")
static class State implements Serializable {
private static final long serialVersionUID = -1825662356883926318L;

// used for up-to-date checks and caching
final String version;
final transient ForeignExe exe;

// used for executing
private transient @Nullable List<String> args;

Expand All @@ -96,10 +100,16 @@ static class State implements Serializable {

String format(ProcessRunner runner, String input, File file) throws IOException, InterruptedException {
if (args == null) {
args = List.of(exe.confirmVersionAndGetAbsolutePath(), "-i", "2", "-ci");
// args will be reused during a single Spotless task execution,
// so this "prefix" is being "cached" for each Spotless format with shfmt.
args = List.of(exe.confirmVersionAndGetAbsolutePath(), "--filename");
}

return runner.exec(input.getBytes(StandardCharsets.UTF_8), args).assertExitZero(StandardCharsets.UTF_8);
// This will ensure that the next file name is retrieved on every format
final List<String> finalArgs = Stream.concat(args.stream(), Stream.of(file.getAbsolutePath()))
.collect(Collectors.toList());

return runner.exec(input.getBytes(StandardCharsets.UTF_8), finalArgs).assertExitZero(StandardCharsets.UTF_8);
}

FormatterFunc.Closeable toFunc() {
Expand Down
3 changes: 3 additions & 0 deletions plugin-gradle/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
### Fixed
* Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))

### Added
* Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))

## [6.25.0] - 2024-01-23
### Added
* Maven / Gradle - Support for formatting Java Docs for the Palantir formatter ([#2009](https://github.com/diffplug/spotless/pull/2009))
Expand Down
6 changes: 5 additions & 1 deletion plugin-gradle/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -992,7 +992,7 @@ spotless {
```gradle
spotless {
shell {
target 'scripts/**/*.sh' // default: '*.sh'
target 'scripts/**/*.sh' // default: '**/*.sh'

shfmt() // has its own section below
}
Expand All @@ -1003,11 +1003,15 @@ spotless {

[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`.
Refer to the `shfmt` [man page](https://github.com/mvdan/sh/blob/master/cmd/shfmt/shfmt.1.scd) for `.editorconfig` settings.
tbcrawford marked this conversation as resolved.
Show resolved Hide resolved

```gradle
shfmt('3.7.0') // version is optional

// if shfmt is not on your path, you must specify its location manually
shfmt().pathToExe('/opt/homebrew/bin/shfmt')

// Spotless always checks the version of the shfmt it is using
// and will fail with an error if it does not match the expected version
// (whether manually specified or default). If there is a problem, Spotless
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

@tbcrawford tbcrawford 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.


static final String NAME = "shell";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,52 @@

@ShfmtTest
public class ShellExtensionTest extends GradleIntegrationHarness {

@Test
void shfmt() throws IOException {
setFile("build.gradle").toLines(
void shfmtWithEditorconfig() throws IOException {
String fileDir = "shell/shfmt/with-config/";

setFile("build.gradle.kts").toLines(
"plugins {",
" id 'com.diffplug.spotless'",
" id(\"com.diffplug.spotless\")",
"}",
"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

" }",
"}");
setFile("shfmt.sh").toResource("shell/shfmt/shfmt.sh");

setFile(".editorconfig").toResource(fileDir + ".editorconfig");

setFile("shfmt.sh").toResource(fileDir + "shfmt.sh");
setFile("scripts/other.sh").toResource(fileDir + "other.sh");

gradleRunner().withArguments("spotlessApply").build();
assertFile("shfmt.sh").sameAsResource("shell/shfmt/shfmt.clean");

assertFile("shfmt.sh").sameAsResource(fileDir + "shfmt.clean");
assertFile("scripts/other.sh").sameAsResource(fileDir + "other.clean");
}

@Test
void shfmtWithoutEditorconfig() throws IOException {
String fileDir = "shell/shfmt/without-config/";

setFile("build.gradle.kts").toLines(
"plugins {",
" id(\"com.diffplug.spotless\")",
"}",
"spotless {",
" shell {",
" shfmt()",
" }",
"}");

setFile("shfmt.sh").toResource(fileDir + "shfmt.sh");
setFile("scripts/other.sh").toResource(fileDir + "other.sh");

gradleRunner().withArguments("spotlessApply").build();

assertFile("shfmt.sh").sameAsResource(fileDir + "shfmt.clean");
assertFile("scripts/other.sh").sameAsResource(fileDir + "other.clean");
}
}
3 changes: 3 additions & 0 deletions plugin-maven/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ We adhere to the [keepachangelog](https://keepachangelog.com/en/1.0.0/) format (
### Fixed
* Ignore system git config when running tests ([#1990](https://github.com/diffplug/spotless/issues/1990))

### Added
* Respect `.editorconfig` settings for formatting shell via `shfmt` ([#2031](https://github.com/diffplug/spotless/pull/2031))

## [2.43.0] - 2024-01-23
### Added
* Support for formatting shell scripts via [shfmt](https://github.com/mvdan/sh). ([#1998](https://github.com/diffplug/spotless/issues/1998))
Expand Down
31 changes: 30 additions & 1 deletion plugin-maven/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1021,11 +1021,40 @@ Uses Jackson and YAMLFactory to pretty print objects:
</jackson>
```

## Shell

- `com.diffplug.spotless.maven.FormatterFactory.addStepFactory(FormatterStepFactory)` [code](./src/main/java/com/diffplug/spotless/maven/shell/Shell.java)

```xml
<configuration>
<shell>
<includes> <!-- Not required. Defaults to **/*.sh -->
<include>scripts/**/*.sh</include>
</includes>

<shfmt /> <!-- has its own section below -->
</shell>
</configuration>
```

### shfmt

[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


```xml
<shfmt>
<version>3.7.0</version> <!-- optional: Custom version of 'mvdan/sh' -->
<pathToExe>/opt/homebrew/bin/shfmt</pathToExe> <!-- optional: if shfmt is not on your path, you must specify its location manually -->
</shfmt>
```

## Gherkin

- `com.diffplug.spotless.maven.FormatterFactory.addStepFactory(FormatterStepFactory)` [code](https://github.com/diffplug/spotless/blob/main/plugin-maven/src/main/java/com/diffplug/spotless/maven/gherkin/Gherkin.java)

```gradle
```xml
<configuration>
<gherkin>
<includes> <!-- You have to set the target manually -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/
package com.diffplug.spotless.maven.shell;

import java.util.Collections;
import java.util.Set;

import org.apache.maven.project.MavenProject;
Expand All @@ -30,9 +29,11 @@
* and shell-specific (e.g. {@link Shfmt}) steps.
*/
public class Shell extends FormatterFactory {
private static final Set<String> DEFAULT_INCLUDES = Set.of("**/*.sh");

@Override
public Set<String> defaultIncludes(MavenProject project) {
return Collections.emptySet();
return DEFAULT_INCLUDES;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.diffplug.spotless.shell.ShfmtStep;

public class Shfmt implements FormatterStepFactory {

@Parameter
private String version;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,36 @@
package com.diffplug.spotless.maven.shell;

import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import com.diffplug.spotless.maven.MavenIntegrationHarness;
import com.diffplug.spotless.tag.ShfmtTest;

@ShfmtTest
public class ShellTest extends MavenIntegrationHarness {
private static final Logger LOGGER = LoggerFactory.getLogger(ShellTest.class);
@Test
public void testFormatShellWithEditorconfig() throws Exception {
String fileDir = "shell/shfmt/with-config/";
setFile("shfmt.sh").toResource(fileDir + "shfmt.sh");
setFile("scripts/other.sh").toResource(fileDir + "other.sh");
setFile(".editorconfig").toResource(fileDir + ".editorconfig");

writePomWithShellSteps("<shfmt/>");
mavenRunner().withArguments("spotless:apply").runNoError();

assertFile("shfmt.sh").sameAsResource(fileDir + "shfmt.clean");
assertFile("scripts/other.sh").sameAsResource(fileDir + "other.clean");
}

@Test
public void testFormatShell() throws Exception {
public void testFormatShellWithoutEditorconfig() throws Exception {
String fileDir = "shell/shfmt/without-config/";
setFile("shfmt.sh").toResource(fileDir + "shfmt.sh");
setFile("scripts/other.sh").toResource(fileDir + "other.sh");

writePomWithShellSteps("<shfmt/>");
setFile("shfmt.sh").toResource("shell/shfmt/shfmt.sh");
mavenRunner().withArguments("spotless:apply").runNoError();
assertFile("shfmt.sh").sameAsResource("shell/shfmt/shfmt.clean");

assertFile("shfmt.sh").sameAsResource(fileDir + "shfmt.clean");
assertFile("scripts/other.sh").sameAsResource(fileDir + "other.clean");
}
}
10 changes: 10 additions & 0 deletions testlib/src/main/resources/shell/shfmt/with-config/.editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
root = true

[*]
charset = utf-8

[*.sh]
indent_style = space
indent_size = 2
space_redirects = true
switch_case_indent = true
20 changes: 20 additions & 0 deletions testlib/src/main/resources/shell/shfmt/with-config/other.clean
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash

fruit="apple"

case $fruit in
"apple")
echo "This is a red fruit."
;;
"banana")
echo "This is a yellow fruit."
;;
"orange")
echo "This is an orange fruit."
;;
*)
echo "Unknown fruit."
;;
esac

echo "This is some text." > output.txt
20 changes: 20 additions & 0 deletions testlib/src/main/resources/shell/shfmt/with-config/other.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash

fruit="apple"

case $fruit in
"apple")
echo "This is a red fruit."
;;
"banana")
echo "This is a yellow fruit."
;;
"orange")
echo "This is an orange fruit."
;;
*)
echo "Unknown fruit."
;;
esac

echo "This is some text." > output.txt
20 changes: 20 additions & 0 deletions testlib/src/main/resources/shell/shfmt/without-config/other.clean
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash

fruit="apple"

case $fruit in
"apple")
echo "This is a red fruit."
;;
"banana")
echo "This is a yellow fruit."
;;
"orange")
echo "This is an orange fruit."
;;
*)
echo "Unknown fruit."
;;
esac

echo "This is some text." >output.txt
Loading
Loading