Skip to content

[MCHECKSTYLE-397] support ${config_loc} on checkstyle xml #36

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.

Sign up for GitHub

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 1 commit into
base: master
Choose a base branch
from
Open

[MCHECKSTYLE-397] support ${config_loc} on checkstyle xml #36

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/main/java/org/apache/maven/plugins/checkstyle/exec/DefaultCheckstyleExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,20 @@ private Properties getOverridingProperties( CheckstyleExecutorRequest request )
}
}

//${config_loc} for the origin dir of the configLocation, just like eclipsecs
// so we config such as `${config_loc}/checkstyle-suppressions.xml`
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

Grammatical error in comment: "so we config such as" should be "so we can configure such as" or "allowing configuration such as".

Suggested change
// so we config such as `${config_loc}/checkstyle-suppressions.xml`
// so we can configure paths such as `${config_loc}/checkstyle-suppressions.xml`

Copilot uses AI. Check for mistakes.
Comment on lines +587 to +588
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

The comment should follow standard Java comment conventions. The variable reference ${config_loc} and the purpose of this code would be clearer with improved documentation. Consider: "Sets the config_loc property to the directory portion of configLocation. This allows Checkstyle configurations to reference the config directory using ${config_loc}, similar to Eclipse CS plugin behavior."

Suggested change
//${config_loc} for the origin dir of the configLocation, just like eclipsecs
// so we config such as `${config_loc}/checkstyle-suppressions.xml`
/*
* Sets the config_loc property to the directory portion of configLocation.
* This allows Checkstyle configurations to reference the config directory using ${config_loc},
* similar to Eclipse CS plugin behavior.
*/

Copilot uses AI. Check for mistakes.
final String configLocation = request.getConfigLocation();
final int idx = configLocation.lastIndexOf( '/' );
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

The code only handles forward slash / as a path separator, but Windows systems use backslash \. This will cause the config_loc property to be incorrectly set on Windows when the config location contains backslashes. Consider using Math.max(configLocation.lastIndexOf('/'), configLocation.lastIndexOf('\\')) to handle both path separators, or use a platform-independent approach.

Suggested change
final int idx = configLocation.lastIndexOf( '/' );
final int idx = Math.max(configLocation.lastIndexOf('/'), configLocation.lastIndexOf('\\'));

Copilot uses AI. Check for mistakes.
final String configLoc;
if ( idx == -1 )
{
configLoc = "";
}
else
{
configLoc = configLocation.substring( 0, idx );
Comment on lines +590 to +598
Copy link

Copilot AI Nov 8, 2025

Choose a reason for hiding this comment

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

Potential NullPointerException: The code doesn't check if configLocation is null before calling lastIndexOf(). While getConfigFile() is currently called before this method in getConfiguration(), this creates a fragile implicit dependency. Add a null check to make the code more robust: if (configLocation != null) { ... }.

Suggested change
final int idx = configLocation.lastIndexOf( '/' );
final String configLoc;
if ( idx == -1 )
{
configLoc = "";
}
else
{
configLoc = configLocation.substring( 0, idx );
final String configLoc;
if (configLocation != null)
{
final int idx = configLocation.lastIndexOf( '/' );
if ( idx == -1 )
{
configLoc = "";
}
else
{
configLoc = configLocation.substring( 0, idx );
}
}
else
{
configLoc = "";

Copilot uses AI. Check for mistakes.
}
p.setProperty( "config_loc", configLoc );
return p;
}

Expand Down
Loading