Skip to content

Fix typo in exceptionVariableName in EmptyCatchBlockCheck #35092

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

Closed
wants to merge 1 commit into from

Conversation

izeye
Copy link
Contributor

@izeye izeye commented Jun 22, 2025

This PR fixes a typo in exceptionVariableName in EmptyCatchBlockCheck.

This PR also changes to use "ignored" for exception variable names instead of "ignore".

See gh-35047

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 22, 2025
This commit also changes to use "ignored" for exception variable names instead of "ignore".

See spring-projectsgh-35047

Signed-off-by: Johnny Lim <[email protected]>
@sbrannen sbrannen added type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 22, 2025
@sbrannen sbrannen self-assigned this Jun 22, 2025
@sbrannen sbrannen added this to the 7.0.0-M7 milestone Jun 22, 2025
@bclozel
Copy link
Member

bclozel commented Jun 22, 2025

Should we apply this?
I configured the check like this because we had already legitimate usage of both "ignore" and "ignored" in the codebase.

I get that "ignore" is not a noun so maybe not very well suited for a variable name. Do we really need to go that far on everything? Should we also refactor the entire codebase to only allow nouns as variable names?

@sbrannen sbrannen removed this from the 7.0.0-M7 milestone Jun 22, 2025
@sbrannen sbrannen added the status: declined A suggestion or change that we don't feel we should currently apply label Jun 22, 2025
@sbrannen
Copy link
Member

sbrannen commented Jun 22, 2025

Should we apply this? I configured the check like this because we had already legitimate usage of both "ignore" and "ignored" in the codebase.

Indeed, it's a regular expression, not a typo.

It's just not immediately obvious from the Checkstyle configuration.

Perhaps changing the pattern to expected|ignore(d)? would make it clearer and more robust.

And perhaps limiting it to only ^expected|ignore(d)?$ would be even better.

I get that "ignore" is not a noun so maybe not very well suited for a variable name. Do we really need to go that far on everything? Should we also refactor the entire codebase to only allow nouns as variable names?

"expected" is also not a noun, so we certainly don't need to enforce any kind of "nouns only" check.

In light of the above, I am closing this PR.

However, we can still consider tightening the RegEx we use for exceptionVariableName to ensure that only "expected", "ignore", or "ignored" is used to permit an empty catch block.

@sbrannen sbrannen closed this Jun 22, 2025
@izeye
Copy link
Contributor Author

izeye commented Jun 22, 2025

@bclozel @sbrannen Thanks for the feedback!

I just thought it would be better to use a consistent name for the same purpose.

@bclozel
Copy link
Member

bclozel commented Jun 22, 2025

Thanks @izeye
The original intent was not clear in the checkstyle configuration.

@izeye izeye deleted the patch-6 branch June 22, 2025 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants