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

Add JSON jackson step, Refactor with Yaml, enable endWithEol, feature… #1492

Closed

Conversation

blacelle
Copy link
Contributor

This improves the work in #1478 and #1446

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.

Looks good! Minor comments below.

@blacelle
Copy link
Contributor Author

With #1478, I got good results regarding mvn cache with ./gradlew :plugin-maven:runMavenBuild :plugin-maven:test --tests '*yaml*' --no-build-cache. Sadly, I again encounter weird issues.

e.g., after modifying a file in the plugin-maven, ./gradlew :plugin-maven:runMavenBuild --no-build-cache --info shows:

> Configure project :plugin-maven
Evaluating project ':plugin-maven' using build file '/Users/blacelle/workspace3/spotless/plugin-maven/build.gradle'.
All projects evaluated.
Task path ':plugin-maven:runMavenBuild' matched project ':plugin-maven'
Task name matched 'runMavenBuild'
Selected primary task 'runMavenBuild' from project :plugin-maven
Tasks to be executed: [task ':plugin-maven:copySourceFiles', task ':plugin-maven:copyMvnw', task ':lib:compileJava', task ':lib:processResources', task ':lib:classes', task ':lib:compileCompatKtLintApiJava', task ':lib:processCompatKtLintApiResources', task ':lib:compatKtLintApiClasses', task ':lib:compileCompatKtLint0Dot31Dot0Java', task ':lib:processCompatKtLint0Dot31Dot0Resources', task ':lib:compatKtLint0Dot31Dot0Classes', task ':lib:compileCompatKtLint0Dot32Dot0Java', task ':lib:processCompatKtLint0Dot32Dot0Resources', task ':lib:compatKtLint0Dot32Dot0Classes', task ':lib:compileCompatKtLint0Dot34Dot2Java', task ':lib:processCompatKtLint0Dot34Dot2Resources', task ':lib:compatKtLint0Dot34Dot2Classes', task ':lib:compileCompatKtLint0Dot45Dot2Java', task ':lib:processCompatKtLint0Dot45Dot2Resources', task ':lib:compatKtLint0Dot45Dot2Classes', task ':lib:compileCompatKtLint0Dot46Dot0Java', task ':lib:processCompatKtLint0Dot46Dot0Resources', task ':lib:compatKtLint0Dot46Dot0Classes', task ':lib:compileCompatKtLint0Dot47Dot0Java', task ':lib:processCompatKtLint0Dot47Dot0Resources', task ':lib:compatKtLint0Dot47Dot0Classes', task ':lib:compileCompatKtLint0Dot48Dot0Java', task ':lib:processCompatKtLint0Dot48Dot0Resources', task ':lib:compatKtLint0Dot48Dot0Classes', task ':lib:compileDiktatJava', task ':lib:compileFlexmarkJava', task ':lib:compileJacksonJava', task ':lib:compileKtfmtJava', task ':lib:compileKtlintJava', task ':lib:compilePalantirJavaFormatJava', task ':lib:compileScalafmtJava', task ':lib:compileSortPomJava', task ':lib:processDiktatResources', task ':lib:diktatClasses', task ':lib:processFlexmarkResources', task ':lib:flexmarkClasses', task ':lib:processJacksonResources', task ':lib:jacksonClasses', task ':lib:processKtfmtResources', task ':lib:ktfmtClasses', task ':lib:processKtlintResources', task ':lib:ktlintClasses', task ':lib:processPalantirJavaFormatResources', task ':lib:palantirJavaFormatClasses', task ':lib:processScalafmtResources', task ':lib:scalafmtClasses', task ':lib:processSortPomResources', task ':lib:sortPomClasses', task ':lib:jar', task ':plugin-maven:install_spotless-lib', task ':lib-extra:compileJava', task ':lib-extra:processResources', task ':lib-extra:classes', task ':lib-extra:jar', task ':plugin-maven:install_spotless-lib-extra', task ':testlib:compileJava', task ':testlib:processResources', task ':testlib:classes', task ':testlib:jar', task ':plugin-maven:install_spotless-testlib', task ':plugin-maven:installLocalDependencies', task ':plugin-maven:createPomXml', task ':plugin-maven:runMavenBuild']
Tasks that were excluded: []
Resolve mutations for :plugin-maven:copySourceFiles (Thread[Execution worker,5,main]) started.
:plugin-maven:copySourceFiles (Thread[Execution worker,5,main]) started.

OK.

Then:

> Task :plugin-maven:copySourceFiles
Caching disabled for task ':plugin-maven:copySourceFiles' because:
  Build cache is disabled
Task ':plugin-maven:copySourceFiles' is not up-to-date because:
  Input property 'rootSpec$1' file /Users/blacelle/workspace3/spotless/plugin-maven/src/main/java/com/diffplug/spotless/maven/json/JacksonJson.java has changed.
Resolve mutations for :plugin-maven:copyMvnw (Thread[Execution worker,5,main]) started.
:plugin-maven:copyMvnw (Thread[Execution worker Thread 14,5,main]) started.

OK.

Then

> Task :plugin-maven:runMavenBuild UP-TO-DATE
Caching disabled for task ':plugin-maven:runMavenBuild' because:
  Build cache is disabled
Skipping task ':plugin-maven:runMavenBuild' as it is up-to-date.

KO : it looks like a change in plugin-maven sourceSets is not considered anymore enough to rebuild the plugin. I do not get how this interacts with --no-build-cache, but I now can get this working only through ./gradlew clean :plugin-maven:runMavenBuild :plugin-maven:test --tests '*yaml*' --no-build-cache which is a pain as the initialization oft he local m2 repo is very slow.

@blacelle
Copy link
Contributor Author

./gradlew :plugin-maven:runMavenBuild --rerun :plugin-maven:test --tests '*yaml*' --no-build-cache does the job. Not being a gradle-friendly user, if this is the legimitate way to test locally a subset of tests from the CLI (as I failed doing so in both Eclipse and IntelliJ), I suppose it should be documented properly.

@blacelle
Copy link
Contributor Author

Now, without any visible changes, I get with the same command:

[INFO] Installing /Users/blacelle/workspace3/spotless/plugin-maven/build/mavenProject/pom.xml to /Users/blacelle/workspace3/spotless/plugin-maven/build/localMavenRepository/com/diffplug/spotless/spotless-maven-plugin/2.30.0-SNAPSHOT/spotless-maven-plugin-2.30.0-SNAPSHOT.pom

  • In plugin 'org.gradle.api.plugins.BasePlugin' type 'org.gradle.api.tasks.bundling.Jar' property '$1' specifies file '/Users/blacelle/workspace3/spotless/plugin-maven/build/mavenProject/target/spotless-maven-plugin-2.31.0-SNAPSHOT.jar' which doesn't exist.

We see something is wrong with the plugin version.

@blacelle
Copy link
Contributor Author

@nedtwigg Ok. Some (magic for newcomers) is going on. I understand some gradle versions are extrapolated from the CHANGES.MD file. As I added some notes in the ## [Unreleased] block, then versions are shuffled and my command is KO. Reverting the maven-plugin/CHANGES.MD fixes the issue. Is it a bug in the gradle config, or in the code extrapolating from a MD?

@nedtwigg
Copy link
Member

My guess is a bug in the maven build. I'm gonna work on #554 to try to solve these at the root.

@nedtwigg
Copy link
Member

I would merge origin feat/maven-build into this PR. #1496 will definitely get merged, and it should fix the issues you're having.

@blacelle
Copy link
Contributor Author

Thanks @nedtwigg . I'm willing to sync by branch as soon as #1496 is merged, and provide some feedback. (If necessary, I can test in before merging, but I suppose we can easily push more improvments regarding to maven-build after the merge).

@blacelle
Copy link
Contributor Author

This introduces jackson-formatters in Gradle, in order to simplify the developement process (referring to maven cache issues (CLI + IDE)).

@blacelle
Copy link
Contributor Author

@nedtwigg I would need a hand. I have issues with com.diffplug.gradle.spotless.YamlExtensionTest#testFormatYaml_WithJackson_defaultConfig_separatorComments. It fails around com.diffplug.spotless.yaml.JacksonYamlStep.State#State as the config is null.

* What went wrong:
Could not determine the dependencies of task ':spotlessApply'.
> Could not create task ':spotlessJsonApply'.
   > Could not create task ':spotlessJson'.
      > ARG

Also why is it referring to spotlessJson?

I've not been able to debug Gradle-based unit-tests through Intellij (I hoped Gradle unit-tests would be easier to test than mvn ones). And even adding --stack-traces and so-on, I can't get a relevant stack-traces (hoping to explain the null JacksonConfig). Thanks

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.

Here's the answer to some of the problems. Hopefully merging main will resolve the rest!

plugin-maven/CHANGES.md Outdated Show resolved Hide resolved
@blacelle
Copy link
Contributor Author

Thanks @nedtwigg , I resynced my branch.

I still get the unexpected null JacksonConfig issue

com.diffplug.gradle.spotless.YamlExtensionTest#testFormatYaml_WithJackson_defaultConfig_separatorComments. It fails around com.diffplug.spotless.yaml.JacksonYamlStep.State#State as the config is null.

but now with the proper step referred i nthe log:

* What went wrong:
Could not determine the dependencies of task ':spotlessApply'.
> Could not create task ':spotlessYamlApply'.
   > Could not create task ':spotlessYaml'.
      > JacksonYamlConfig must not be null

@blacelle
Copy link
Contributor Author

blacelle commented Jan 18, 2023

With the new mvn-build setup, running tests in Intellij has a new faulty behavior:

image

> Task :lib:javadoc FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':lib:javadoc'.
> Javadoc generation failed. Generated Javadoc options file (useful for troubleshooting): '/Users/blacelle/workspace3/spotless/lib/build/tmp/javadoc/javadoc.options'

While it is awkward to have unittests broken by Javadoc, fixes in the Javadoc (necessary to fix the whole build anyway) enables testing of mvn tests from Intellij. Excellent.

@blacelle
Copy link
Contributor Author

blacelle commented Jan 18, 2023

The issues related with null jackson config were due to JacksonYamlGradleConfig extending AJacksonGradleConfig, and createStep was relying a the extended class field. Noob issue, but too bad it was difficult to get a stack (I ended with Throwables.getStackTraceAsString(new RuntimeException())).

Note for gradle tests: one can easily add stacktraces with:

gradleRunner().withArguments("spotlessApply", "--stacktrace").build();

@blacelle
Copy link
Contributor Author

./gradlew :plugin-maven:test --tests '*json*'

and

./gradlew :plugin-maven:test --tests '*yaml*'

Run smoothly. That's great to facilitate mvn-plugin testing from CLI.

@blacelle blacelle marked this pull request as ready for review January 18, 2023 10:16
@blacelle
Copy link
Contributor Author

@nedtwigg This is ready for review.

@blacelle
Copy link
Contributor Author

blacelle commented Jan 18, 2023

We followed suggestion from FasterXML/jackson-dataformats-text#66

@nedtwigg
Copy link
Member

This looks great, except the table in the root README.md needs to get updated (new name for Yaml, check gradle, add JacksonJsonStep). I did this locally, but I can't push to your branch. It's not a big deal, but if possible it would be better if you submit PR's from blacelle/spotless, as it seems that solven-eu does not allow me to help with PRs.

@blacelle
Copy link
Contributor Author

Moved into #1508

@blacelle blacelle closed this Jan 19, 2023
@blacelle blacelle deleted the ProgressWithJackson_YamlAndJson branch January 19, 2023 18:38
@nedtwigg
Copy link
Member

Released in plugin-gradle 6.14.0 and plugin-maven 2.31.0.

benkard added a commit to benkard/mulkcms2 that referenced this pull request Apr 2, 2023
This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [com.diffplug.spotless:spotless-maven-plugin](https://github.com/diffplug/spotless) | build | minor | `2.32.0` -> `2.33.0` |
| [io.quarkus:quarkus-maven-plugin](https://github.com/quarkusio/quarkus) | build | patch | `2.16.1.Final` -> `2.16.2.Final` |
| [io.quarkus:quarkus-universe-bom](https://github.com/quarkusio/quarkus-platform) | import | patch | `2.16.1.Final` -> `2.16.2.Final` |

---

### Release Notes

<details>
<summary>diffplug/spotless</summary>

### [`v2.33.0`](https://github.com/diffplug/spotless/blob/HEAD/CHANGES.md#&#8203;2330---2023-01-26)

##### Added

-   `ProcessRunner` has added some convenience methods so it can be used for maven testing. ([#&#8203;1496](diffplug/spotless#1496))
-   `ProcessRunner` allows to limit captured output to a certain number of bytes. ([#&#8203;1511](diffplug/spotless#1511))
-   `ProcessRunner` is now capable of handling long-running tasks where waiting for exit is delegated to the caller. ([#&#8203;1511](diffplug/spotless#1511))
-   Allow to specify node executable for node-based formatters using `nodeExecutable` parameter ([#&#8203;1500](diffplug/spotless#1500))

##### Fixed

-   The default list of type annotations used by `formatAnnotations` has had 8 more annotations from the Checker Framework added [#&#8203;1494](diffplug/spotless#1494)

##### Changes

-   **POTENTIALLY BREAKING** Bump minimum JRE from 8 to 11, next release likely to bump bytecode to Java 11 ([#&#8203;1514](diffplug/spotless#1514) part 1 of [#&#8203;1337](diffplug/spotless#1337))
-   Rename `YamlJacksonStep` into `JacksonYamlStep` while normalizing Jackson usage ([#&#8203;1492](diffplug/spotless#1492))
-   Convert `gson` integration to use a compile-only source set ([#&#8203;1510](diffplug/spotless#1510)).
-   \*\* POTENTIALLY BREAKING\*\* Removed support for KtLint 0.3x and 0.45.2 ([#&#8203;1475](diffplug/spotless#1475))
    -   `KtLint` does not maintain a stable API - before this MR, we supported every breaking change in the API since 2019.
    -   From now on, we will support no more than 2 breaking changes at a time.
-   NpmFormatterStepStateBase delays `npm install` call until the formatter is first used. This enables better integration
    with `gradle-node-plugin`. ([#&#8203;1522](diffplug/spotless#1522))
-   Bump default `ktlint` version to latest `0.48.1` -> `0.48.2` ([#&#8203;1529](diffplug/spotless#1529))
-   Bump default `scalafmt` version to latest `3.6.1` -> `3.7.1` ([#&#8203;1529](diffplug/spotless#1529))

</details>

<details>
<summary>quarkusio/quarkus</summary>

### [`v2.16.2.Final`](https://github.com/quarkusio/quarkus/releases/tag/2.16.2.Final)

[Compare Source](quarkusio/quarkus@2.16.1.Final...2.16.2.Final)

##### Complete changelog

-   [#&#8203;30976](quarkusio/quarkus#30976) - Metrics - check if index contains class before attempting to use it
-   [#&#8203;30965](quarkusio/quarkus#30965) - JandexBeanInfoAdapter.getMetricAnnotationsThroughStereotype is not null safe
-   [#&#8203;30959](quarkusio/quarkus#30959) - Return text from /q/metrics when the Accept header contains html
-   [#&#8203;30953](quarkusio/quarkus#30953) - Fix OIDC capability string
-   [#&#8203;30947](quarkusio/quarkus#30947) - Ignore interface/class without default constructs fields in SB config
-   [#&#8203;30940](quarkusio/quarkus#30940) - Use SchemaType.ARRAY instead of "ARRAY" for native support
-   [#&#8203;30919](quarkusio/quarkus#30919) - Compilation to native fails, when quarkus-smallrye-openapi is included
-   [#&#8203;30916](quarkusio/quarkus#30916) - Add AppCDS documentation
-   [#&#8203;30896](quarkusio/quarkus#30896) - Quarkus spring-boot-properties extension unable to handle complex configuration.
-   [#&#8203;30878](quarkusio/quarkus#30878) - Bump postgresql from 42.5.2 to 42.5.3
-   [#&#8203;30866](quarkusio/quarkus#30866) - Only run the quickstart compilation for main
-   [#&#8203;30851](quarkusio/quarkus#30851) - Fixed return type typo in smallrye graphQL guide
-   [#&#8203;30844](quarkusio/quarkus#30844) - Fixed greeting in getting started guide
-   [#&#8203;30839](quarkusio/quarkus#30839) - Fix handling of Accept header in graphQL
-   [#&#8203;30833](quarkusio/quarkus#30833) - Update docs to show BuildProducer use as method parameter instead of field
-   [#&#8203;30828](quarkusio/quarkus#30828) - Make OIDC session cookie same site lax by default
-   [#&#8203;30826](quarkusio/quarkus#30826) - Caffeine - Automatically register metrics cache impls if Micrometer is around
-   [#&#8203;30825](quarkusio/quarkus#30825) - Fix comment about Caffeine optimization
-   [#&#8203;30823](quarkusio/quarkus#30823) - Change accept header to valid plain text in micrometer documentation
-   [#&#8203;30821](quarkusio/quarkus#30821) - Packaging type -Dquarkus.package.create-appcds=true isn't documented
-   [#&#8203;30815](quarkusio/quarkus#30815) - Update SmallRye Config to 2.13.2
-   [#&#8203;30812](quarkusio/quarkus#30812) - Manage the apache-mime4j dependency
-   [#&#8203;30806](quarkusio/quarkus#30806) - */* in Accept header is ignored if not listed as the first item
-   [#&#8203;30805](quarkusio/quarkus#30805) - MailTemplateInstance with attachments
-   [#&#8203;30803](quarkusio/quarkus#30803) - Support file and byte array attachments in `MailTemplateInstance`
-   [#&#8203;30797](quarkusio/quarkus#30797) - OIDC login not work
-   [#&#8203;30783](quarkusio/quarkus#30783) - <artifactId> uses 'quarkus.platform.artifact-id' property
-   [#&#8203;30778](quarkusio/quarkus#30778) - Avoid creating 3 Liquibase MongoDB instances for startup operations
-   [#&#8203;30776](quarkusio/quarkus#30776) - Ensure that AwsProxyRequestContext can be used with [@&#8203;Context](https://github.com/Context) in RESTEasy Reactive
-   [#&#8203;30767](quarkusio/quarkus#30767) - Remove duplicate notification of SseBroadcaster's onErrorListeners
-   [#&#8203;30765](quarkusio/quarkus#30765) - Bump postgresql from 42.5.1 to 42.5.2
-   [#&#8203;30755](quarkusio/quarkus#30755) - Update ForwardedParser to validate the port
-   [#&#8203;30744](quarkusio/quarkus#30744) - \[Quarkus Native] ClassNotFoundException: com.github.benmanes.caffeine.cache.SSSW
-   [#&#8203;30536](quarkusio/quarkus#30536) - munitnyucontextmanager non helpful error reporting
-   [#&#8203;29753](quarkusio/quarkus#29753) - Introduce ConnectionFactoryWrapperBuildItem
-   [#&#8203;29605](quarkusio/quarkus#29605) - Update docs to reflect that injection should not
-   [#&#8203;27774](quarkusio/quarkus#27774) - PLANNER-1709 Avoid deprecated penalize/reward overloads
-   [#&#8203;23442](quarkusio/quarkus#23442) - problem using quarkus-resteasy-reactive-kotlin-serialization with AwsProxyRequestContext

</details>

<details>
<summary>quarkusio/quarkus-platform</summary>

### [`v2.16.2.Final`](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final)

[Compare Source](quarkusio/quarkus-platform@2.16.1.Final...2.16.2.Final)

</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