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

Issue #13345: enable example test for SeverityMatchFilterExamplesTest #14784

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kexin2000
Copy link
Contributor

part of issue: #13345
this is the first example test for filter, I've encountered some issues and need to discuss potential solutions. @romani @nrmancuso

  1. My current approach to writing filter examples is inspired by this file: demonstrating the function of the filter through other checks. I used ParameterNameCheck and MethodNameCheck, setting the severity of ParameterNameCheck to info, while the MethodNameCheck defaults to error. However, when I run SeverityMatchFilterExamplesTest, the code enters line 382 of the following code block and throws an exception. The reason is that the path where SeverityMatchFilterExamplesTest is located does not contain module name parameters for 'parametername' and 'methodname'. So, I added 'parametername' and 'methodname' to the hashmap 'moduleMappings'. I think this approach should be acceptable, as I used 'git blame' to investigate the historical reason for 'moduleMappings'. The existence of this variable is to address some special cases in Enable examples tests #13345, such as SuppressWarningsHolder SuppressWarningsHolder naming inconsistency giving java.lang.ClassNotFoundException #13845.

String fullyQualifiedClassName;
if (moduleMappings.containsKey(moduleName)) {
fullyQualifiedClassName = moduleMappings.get(moduleName);
}
else if (moduleName.startsWith("com.")) {
fullyQualifiedClassName = moduleName;
}
else {
final String path = SLASH_PATTERN.matcher(filePath).replaceAll(".");
final int endIndex = path.lastIndexOf(moduleName.toLowerCase(Locale.ROOT));
if (endIndex == -1) {
throw new CheckstyleException("Unable to resolve module name: " + moduleName
+ ". Please check for spelling errors or specify fully qualified class name.");
}
final int beginIndex = path.indexOf("com.puppycrawl");
fullyQualifiedClassName = path.substring(beginIndex, endIndex) + moduleName;
if (!fullyQualifiedClassName.endsWith("Filter")) {
fullyQualifiedClassName += "Check";
}

  1. While running SeverityMatchFilterExamplesTest, there's a similar issue with a path mismatch. The code will throw an exception at line 602 of the following code block because there is no message in the filters package. So, I added a messages.properties file under src/main/resources/com/puppycrawl/tools/checkstyle/filters, and for subsequent filter example tests, more violation messages can be added to this file to resolve this issue. Is my solution feasible? (Messages.properties files for other languages also need to be added.)

private static String internalGetCheckMessage(
String messageBundle, String messageKey, Object... arguments) {
final ResourceBundle resourceBundle = ResourceBundle.getBundle(
messageBundle,
Locale.ROOT,
Thread.currentThread().getContextClassLoader(),
new Utf8Control());
final String pattern = resourceBundle.getString(messageKey);
final MessageFormat formatter = new MessageFormat(pattern, Locale.ROOT);
return formatter.format(arguments);
}

@Kexin2000
Copy link
Contributor Author

The work on this PR is not yet complete. To avoid unnecessary work, I want to first confirm a suitable solution.

Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

items:

src/xdocs/filters/severitymatchfilter.xml Show resolved Hide resolved
@@ -0,0 +1 @@
name.invalidPattern=Name ''{0}'' must match pattern ''{1}''.
Copy link
Member

Choose a reason for hiding this comment

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

what forcing you to add this file and such property?

Copy link
Member

Choose a reason for hiding this comment

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

see guidance at #14784 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@romani
Copy link
Member

romani commented Apr 11, 2024

So, I added 'parametername' and 'methodname' to the hashmap 'moduleMappings'. I think this approach should be acceptable,

yes, it is defect of our tes suite as it expects modules to be in same package and target module - Filter in this case.

So, I added a messages.properties file under src/main/resources/com/puppycrawl/tools/checkstyle/filters, and for subsequent filter example tests, more violation messages can be added to this file to resolve this issue. Is my solution feasible?

not good, can we update internal suite code to take messages from file we will provide as hardcoded values, same as we did for fully qualified names ( item above)

@Kexin2000 Kexin2000 force-pushed the enable_SeverityMatchFilterExamplesTest branch from e14b1a8 to f9f877a Compare April 11, 2024 18:57
@Kexin2000 Kexin2000 requested a review from romani April 11, 2024 19:00
Copy link
Member

@romani romani left a comment

Choose a reason for hiding this comment

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

Items


public void method1(int V1){} // ok, ParameterNameCheck's severity is info

public void Method2(){} // violation
Copy link
Member

Choose a reason for hiding this comment

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

// violation, as default severity is error

<source>
public class Example1 {

public void method1(int V1){} // ok, ParameterNameCheck's severity is info
Copy link
Member

Choose a reason for hiding this comment

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

// ok, ParameterName's severity is info

if (endIndex < 0) {
messageBundle = messages;
}
else {
final String packageName = className.substring(0, endIndex);
messageBundle = packageName + "." + messages;
if ("com.puppycrawl.tools.checkstyle.filters".equals(packageName)) {
messageBundle = messageBundleMappings.get(className.substring(endIndex + 1));
Copy link
Member

Choose a reason for hiding this comment

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

Please change logic to use value from manual mapping if mapping is present.
If present in special map then use it , else use packageName

@romani
Copy link
Member

romani commented May 11, 2024

@Kexin2000 ,please find time to finish PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants