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

Allow skipping targets based on text they contain #1749

Closed
wants to merge 2 commits into from
Closed

Allow skipping targets based on text they contain #1749

wants to merge 2 commits into from

Conversation

oldergod
Copy link
Contributor

@oldergod oldergod commented Jul 8, 2023

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.

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

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

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

I would

Now, in FormatExtension, save the excludeContent, and apply it to each step at the end with steps.replaceAll

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

Copy link
Contributor Author

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.

Comment on lines 242 to 245
for (String skipIfContentContain : skipIfContentContains) {
if (unix.contains(skipIfContentContain))
return unix;
}
Copy link
Contributor Author

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.

@oldergod oldergod changed the title Bquenaudon.2023 07 08.filterfileconsuming Allow skipping targets based on text they contain Jul 8, 2023
Copy link
Member

@nedtwigg nedtwigg left a 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.

Comment on lines 56 to +58
public default FormatterStep filterByContentPattern(String contentPattern) {
return new FilterByContentPatternFormatterStep(this, contentPattern);
return filterByContentPattern(contentPattern, true);
}
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 can update this one directly if you don't mind having an API change.

@nedtwigg
Copy link
Member

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.

@nedtwigg nedtwigg closed this Jul 14, 2023
@oldergod oldergod deleted the bquenaudon.2023-07-08.filterfileconsuming branch July 14, 2023 18:16
@oldergod
Copy link
Contributor Author

Thank you

@nedtwigg
Copy link
Member

Released in plugin-gradle 6.20.0.

benkard pushed a commit to benkard/quarkus-googlecloud-jsonlogging that referenced this pull request Sep 30, 2023
…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#&#8203;2400---2023-07-17)

##### Added

-   Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#&#8203;1208](diffplug/spotless#1208))
-   `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#&#8203;1749](diffplug/spotless#1749))

##### Fixed

-   Update documented default `semanticSort` to `false`. ([#&#8203;1728](diffplug/spotless#1728))

##### Changes

-   Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#&#8203;1734](diffplug/spotless#1734))
-   Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#&#8203;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-->
benkard pushed a commit to benkard/mulkcms2 that referenced this pull request Nov 12, 2023
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#&#8203;2400---2023-07-17)

##### Added

-   Added support for Protobuf formatting based on [Buf](https://buf.build/). ([#&#8203;1208](diffplug/spotless#1208))
-   `enum OnMatch { INCLUDE, EXCLUDE }` so that `FormatterStep.filterByContent` can not only include based on the pattern but also exclude. ([#&#8203;1749](diffplug/spotless#1749))

##### Fixed

-   Update documented default `semanticSort` to `false`. ([#&#8203;1728](diffplug/spotless#1728))

##### Changes

-   Bump default `cleanthat` version to latest `2.13` -> `2.17`. ([#&#8203;1734](diffplug/spotless#1734))
-   Bump default `ktlint` version to latest `0.49.1` -> `0.50.0`. ([#&#8203;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-->
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.

2 participants