Skip to content

feat: add execution ID parameter to failing spotless check log #2483

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dpolivaev
Copy link

@dpolivaev dpolivaev commented May 20, 2025

for improved error messaging, issue #1988

  • Introduced a new parameter executionId to capture the execution ID from Maven's configuration.
  • Updated error messages to include the execution ID when suggesting commands to fix format violations.
  • Enhanced test cases to validate the new behavior of error messages based on the execution ID.

…ror messaging, issue diffplug#1988

- Introduced a new parameter `executionId` to capture the execution ID from Maven's configuration.
- Updated error messages to include the execution ID when suggesting commands to fix format violations.
- Enhanced test cases to validate the new behavior of error messages based on the execution ID.
@dpolivaev dpolivaev changed the title feat: add execution ID parameter to SpotlessCheckMojo feat: add execution ID parameter to failing spotless check log May 20, 2025
* The execution ID from Maven's configuration.
*/
@Parameter(defaultValue = "${mojoExecution.executionId}")
private String executionId;
Copy link
Contributor

Choose a reason for hiding this comment

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

The execution ids of different goals usually differ as well!

Copy link
Author

@dpolivaev dpolivaev May 20, 2025

Choose a reason for hiding this comment

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

This is how we use it:

    <profile>
      <id>palantir-java-format-profile</id>
      <activation>
        <jdk>[11,)</jdk>
      </activation>
      <build>
        <plugins>
          <plugin>
            <groupId>com.diffplug.spotless</groupId>
            <artifactId>spotless-maven-plugin</artifactId>
            <executions>
              <execution>
                <!--
                Run "mvn spotless:check@palantir-java-format" to check the
                formatting outside of the "mvn verify" phase.

                Similar: To format your code, run "mvn spotless:apply@palantir-java-format".

                You need to add the execution id with "@palantir-java-format" to your comand,
                so spotless will find its configuration containing the palantirJavaFormat.
                -->
                <id>palantir-java-format</id>
                <phase>verify</phase>
                <goals>
                  <goal>check</goal>
                </goals>
                <configuration>
                  <skip>${spotless.skip.palantirJavaFormat}</skip>
                  <java>
                    <palantirJavaFormat/>
                  </java>
                </configuration>
              </execution>
            </executions>
          </plugin>
        </plugins>
      </build>
    </profile>

Therefore we do not need different configurations and different execution IDs for the different goals (check and apply). We don't repeat ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

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

They can be the same but don’t have to. With the default assuming the same there is a high chance something is emitted which does not work. I would leave out the default value for this parameter

Copy link
Author

Choose a reason for hiding this comment

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

Do you think we need an optional separate parameter with a dedicated value? How would you name it?

The whole idea is to output correct command line automatically. Currently it outputs just run 'mvn spotless:apply' and it was the reason why I created the pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think just changing the output inncase a dedicated execution id is configured is totally sufficient

…Mojo

- Updated the logic to check for execution IDs, ensuring that messages correctly suggest commands based on the presence of default execution IDs.
- Added a new test case to validate behavior when no explicit execution ID is provided, confirming that the output message is simplified accordingly.
@dpolivaev
Copy link
Author

dpolivaev commented May 21, 2025

Maven always adds an execution id even if it is not configured in the pom file.
I used a debugger and implemented a heuristic better separating user specified execution ids from maven generated ones.
Now I assume execution id "default" or starting with "default-" like "default-cli" to be automatically added and do not mention them in the generated output.
Unfortunately, I have not found a direct access to the xml after it was processed by the maven xml parser because the parser does not generate any DOM.

@nedtwigg
Copy link
Member

This needs a spotlessApply, but once CI passes this LGTM. Before I merge, I'd like one of our established maven contributors to give it a thumbs-up first. I am not maven-literate-enough to understand the debate above. I'll publish a set of releases once this either gets merged or rejected.

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.

4 participants