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 #14715: Enforce new naming convention on RecordTypeParameterName in IT area #14727

Merged

Conversation

Lmh-java
Copy link
Contributor

@Lmh-java Lmh-java commented Mar 28, 2024

Part of #14715

@Lmh-java Lmh-java force-pushed the minghao/xpath-naming-convention-record branch from 2f47b39 to 5128c8b Compare March 28, 2024 05:28
@Lmh-java Lmh-java changed the title #14715: Enforce new naming convention on RecordTypeParameterName in IT area Issue #14715: Enforce new naming convention on RecordTypeParameterName in IT area Mar 28, 2024
Copy link
Contributor

@MANISH-K-07 MANISH-K-07 left a comment

Choose a reason for hiding this comment

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

IMO, it would be better if the record names were same as filenames just like we do with classes by convention.

Items :

@@ -41,7 +41,7 @@ protected String getCheckName() {
@Test
public void testOne() throws Exception {
final File fileToProcess = new File(getNonCompilablePath(
"SuppressionXpathRegressionRecordTypeParameterName1.java"));
"InputXpathRecordTypeParameterNameExtend.java"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"InputXpathRecordTypeParameterNameExtend.java"));
"InputXpathRecordTypeParameterNameTypeDeclared.java"));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see the point why you would name it like that. Could you explain it a bit? Basically, why would you add "TypeXXXX" to the file name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test is testing the generic var with extends and & synatx.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please read about TypeVariables in java.
https://docs.oracle.com/javase%2F7%2Fdocs%2Fapi%2F%2F/java/lang/reflect/TypeVariable.html

Make changes if you feel my suggestions make more sense than suffixes "Extend" and "Default"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late reply, done

@@ -67,7 +67,7 @@ public void testOne() throws Exception {
@Test
public void testTwo() throws Exception {
final File fileToProcess = new File(getNonCompilablePath(
"SuppressionXpathRegressionRecordTypeParameterName2.java"));
"InputXpathRecordTypeParameterNameDefault.java"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"InputXpathRecordTypeParameterNameDefault.java"));
"InputXpathRecordTypeParameterNameTypeDefault.java"));

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

@Lmh-java Lmh-java force-pushed the minghao/xpath-naming-convention-record branch from 5128c8b to 0ba217f Compare March 30, 2024 05:25
@rnveach
Copy link
Member

rnveach commented Apr 2, 2024

@MANISH-K-07 I assume you are done?

Copy link
Contributor

@MANISH-K-07 MANISH-K-07 left a comment

Choose a reason for hiding this comment

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

Yes @rnveach ,
I think everything looks good here but I'm not in the power to approve so please approve if you agree with the suggestions..

@rnveach rnveach merged commit 645a1db into checkstyle:master Apr 2, 2024
113 checks passed
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.

None yet

3 participants