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 #6207: Add Xpath regression test for LocalFinalVariableName #14639

Conversation

Lmh-java
Copy link
Contributor

Part of issue #6207

@Lmh-java Lmh-java marked this pull request as ready for review March 10, 2024 18:08
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.

@Lmh-java ,
I think it would be better if we cover one input for Ctors and class fields
Can you can manage this ?

@Lmh-java
Copy link
Contributor Author

Sure, I can do Ctors, but can you elaborate more on class fields? I don't think we need class fields here, since this is the check for final local variables inside a method (correct me if I am wrong)

@MANISH-K-07
Copy link
Contributor

Thanks for the reply.. otherwise I wouldn't have gone through the check now ;)
Please hold on until we get a bit of clarity....

@romani ,
Could you please tell me why LocalFinalVariableNameCheck is only testing for three tokens (basically just parameters and try-with-resources block).
I believe this should apply to final variables in Ctors as well? Infact, it should apply to any block in general ??
Please correct me in case my understanding of this check is wrong 😅

@Lmh-java
Copy link
Contributor Author

Lmh-java commented Mar 10, 2024

I took a close look at the checker. Here is a screenshot of the class-level comment.
image
So it is only checking VARIABLE_DEF, PARAMETER_DEF, and RESOURCE.
The checker also re-writes mustCheckName(DetailAST) to only focus on local variables. The method detects variables declared in "a code block, a for initializer, or a catch parameter".

Therefore, I think I might need to include extra tests for:

  • final variables declaration inside a for initializer.
  • parameters of a method that are marked as final.
    And these tests would have different Xpath structures.

Hi @MANISH-K-07 , what's your opinion about this?

@MANISH-K-07
Copy link
Contributor

MANISH-K-07 commented Mar 10, 2024

Hi @MANISH-K-07 , what's your opinion about this?

@Lmh-java ,
Like I mentioned in my post above, You are partially right though, That is what the description says but I'm not sure what the Check is actually testing. I will run a few test cases as soon as I get on my laptop, and get back with answers !!

@romani ,
Could you please tell me why LocalFinalVariableNameCheck is only testing for three tokens (basically just parameters and try-with-resources block).
I believe this should apply to final variables in Ctors as well? Infact, it should apply to any block in general ??
Please correct me in case my understanding of this check is wrong 😅

Meanwhile, Let's wait for @romani to have a look at my query at #14639 (comment)
;)

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.

Ok to merge.

This Check only for local variables , not parameters.
We should have separate Check to cover parameters

@romani romani merged commit bbb28a7 into checkstyle:master Mar 15, 2024
112 of 113 checks passed
@Lmh-java Lmh-java deleted the minghao/xpath-regression-test-local-final-var branch March 15, 2024 16:27
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