-
Notifications
You must be signed in to change notification settings - Fork 477
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
base: main
Are you sure you want to change the base?
Conversation
…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.
…r format violations (diffplug#1988)
* The execution ID from Maven's configuration. | ||
*/ | ||
@Parameter(defaultValue = "${mojoExecution.executionId}") | ||
private String executionId; |
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 execution ids of different goals usually differ as well!
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.
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.
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.
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
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.
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.
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 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.
Maven always adds an execution id even if it is not configured in the pom file. |
This needs a |
for improved error messaging, issue #1988
executionId
to capture the execution ID from Maven's configuration.