-
Notifications
You must be signed in to change notification settings - Fork 455
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
Allow skipping targets based on text they contain #1749
Allow skipping targets based on text they contain #1749
Conversation
@@ -45,14 +45,17 @@ public final class Formatter implements Serializable, AutoCloseable { | |||
private Charset encoding; | |||
private Path rootDir; | |||
private List<FormatterStep> steps; | |||
private List<String> skipIfContentContains; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't add it as a step because every steps would then be repeating the same logic. I thought it best to filter files before thinking of computing them or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach you took is a bit faster, but it has some downsides
- more code (we already have a mechanism for skipping based on content, now we have two mechanisms)
- functionality spread (formatter was as simple as possible to operate the FormatterStep, now it has one more mechanism that it needs to have)
This is a useful feature to have, but I think it is better to implement it using FormatterStep.filterByContentPattern
. Making the fast part of a program a little faster doesn't matter very much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. I removed most of the code and left the API in the Gradle extension + the tests.
I don't see how to use FilterByContentPatternFormatterStep
so I took it out. This one is opting-in based on a pattern, and I would wanna do the reverse. I don't know how to do that from the existing class.
If this was sorted out, I would probably then, when setting the task, wrap all steps with a filtering one.
Let me know if you have better ideas on how to reuse FilterByContentPatternFormatterStep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would
- add a field
boolean formatIfMatches
tospotless/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java
Lines 25 to 29 in d245b60
final class FilterByContentPatternFormatterStep extends DelegateFormatterStep { final Pattern contentPattern; FilterByContentPatternFormatterStep(FormatterStep delegateStep, String contentPattern) { super(delegateStep); - then change this to
if (matcher.find() == formatIfMatches)
spotless/lib/src/main/java/com/diffplug/spotless/FilterByContentPatternFormatterStep.java
Lines 25 to 29 in d245b60
final class FilterByContentPatternFormatterStep extends DelegateFormatterStep { final Pattern contentPattern; FilterByContentPatternFormatterStep(FormatterStep delegateStep, String contentPattern) { super(delegateStep); - Now you can add a
boolean
to this methodspotless/lib/src/main/java/com/diffplug/spotless/FormatterStep.java
Lines 56 to 58 in d245b60
public default FormatterStep filterByContentPattern(String contentPattern) { return new FilterByContentPatternFormatterStep(this, contentPattern); }
Now, in FormatExtension
, save the excludeContent
, and apply it to each step at the end with steps.replaceAll
spotless/plugin-gradle/src/main/java/com/diffplug/gradle/spotless/FormatExtension.java
Lines 883 to 892 in d245b60
List<FormatterStep> steps; | |
if (togglePair != null) { | |
steps = new ArrayList<>(this.steps.size() + 2); | |
steps.add(togglePair.in()); | |
steps.addAll(this.steps); | |
steps.add(togglePair.out()); | |
} else { | |
steps = this.steps; | |
} | |
task.setSteps(steps); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the code @nedtwigg with passing tests.
for (String skipIfContentContain : skipIfContentContains) { | ||
if (unix.contains(skipIfContentContain)) | ||
return unix; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic might be executed twice across computeLineEndings
and compute
but they're both public so to guarantee skipIfContentContains
are respected, I added it to both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatter
should not change, use FormatterStep.filterByContentPattern
.
public default FormatterStep filterByContentPattern(String contentPattern) { | ||
return new FilterByContentPatternFormatterStep(this, contentPattern); | ||
return filterByContentPattern(contentPattern, true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can update this one directly if you don't mind having an API change.
Because I'm not able to push to this PR, I opened a new PR (#1755) where I added a few more commits. Thanks for a great PR, it will be merged shortly. |
Thank you |
Released in |
…lugin to v2.40.0 (mulk/quarkus-googlecloud-jsonlogging!20) This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.39.0` -> `2.40.0` | --- ### Release Notes <details> <summary>diffplug/spotless</summary> ### [`v2.40.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2400---2023-07-17) ##### Added - Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#​1208](diffplug/spotless#1208)) - `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#​1749](diffplug/spotless#1749)) ##### Fixed - Update documented default `semanticSort` to `false`. ([#​1728](diffplug/spotless#1728)) ##### Changes - Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#​1734](diffplug/spotless#1734)) - Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#​1741](diffplug/spotless#1741)) - Dropped support for `ktlint 0.47.x` following our policy of supporting two breaking changes at a time. - Dropped support for deprecated `useExperimental` parameter in favor of the `ktlint_experimental` property. </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [flow-bin](https://github.com/flowtype/flow-bin) ([changelog](https://github.com/facebook/flow/blob/master/Changelog.md)) | devDependencies | minor | [`^0.216.0` -> `^0.217.0`](https://renovatebot.com/diffs/npm/flow-bin/0.216.1/0.217.2) | | [org.liquibase:liquibase-maven-plugin](http://www.liquibase.org/liquibase-maven-plugin) ([source](https://github.com/liquibase/liquibase)) | build | minor | `4.23.2` -> `4.24.0` | | [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.39.0` -> `2.40.0` | | [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `3.4.1` -> `3.4.2` | | [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `3.4.1` -> `3.4.2` | --- ### Release Notes <details> <summary>flowtype/flow-bin</summary> ### [`v0.217.2`](flow/flow-bin@15ccd14...dc93913) [Compare Source](flow/flow-bin@15ccd14...dc93913) ### [`v0.217.1`](flow/flow-bin@6af43b3...15ccd14) [Compare Source](flow/flow-bin@6af43b3...15ccd14) ### [`v0.217.0`](flow/flow-bin@f96ca32...6af43b3) [Compare Source](flow/flow-bin@f96ca32...6af43b3) </details> <details> <summary>liquibase/liquibase</summary> ### [`v4.24.0`](https://github.com/liquibase/liquibase/blob/HEAD/changelog.txt#Liquibase-4240-is-a-major-release) [Compare Source](liquibase/liquibase@v4.23.2...v4.24.0) </details> <details> <summary>diffplug/spotless</summary> ### [`v2.40.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#​2400---2023-07-17) ##### Added - Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#​1208](diffplug/spotless#1208)) - `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#​1749](diffplug/spotless#1749)) ##### Fixed - Update documented default `semanticSort` to `false`. ([#​1728](diffplug/spotless#1728)) ##### Changes - Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#​1734](diffplug/spotless#1734)) - Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#​1741](diffplug/spotless#1741)) - Dropped support for `ktlint 0.47.x` following our policy of supporting two breaking changes at a time. - Dropped support for deprecated `useExperimental` parameter in favor of the `ktlint_experimental` property. </details> <details> <summary>quarkusio/quarkus</summary> ### [`v3.4.2`](quarkusio/quarkus@3.4.1...3.4.2) [Compare Source](quarkusio/quarkus@3.4.1...3.4.2) </details> <details> <summary>quarkusio/quarkus-platform</summary> ### [`v3.4.2`](quarkusio/quarkus-platform@3.4.1...3.4.2) [Compare Source](quarkusio/quarkus-platform@3.4.1...3.4.2) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 👻 **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Follow-up of #1738
Add an field to FormatExtension (Gradle) to accept lists of text which if detected will get a Formatter to skip a file as a whole and will not apply anything.
I didn't touch the
maven
plugin.