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 #14825: fix false positive for WhitespaceAroundCheck #14858

Merged
merged 1 commit into from May 13, 2024

Conversation

strkkk
Copy link
Member

@strkkk strkkk commented May 5, 2024

|| isArrayInitialization(currentType, parentType);
}
}
return result;
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to refactor this method since complexity was >10 and caused violation

@nrmancuso
Copy link
Member

Hi @strkkk :) please generate a regression report when you get a chance, you can assign me to this PR when you are ready.

@strkkk
Copy link
Member Author

strkkk commented May 7, 2024

Github, generate report

@romani
Copy link
Member

romani commented May 7, 2024

No violations on this by this fix: switch' is not followed by whitespace. var otherSuffix = switch(locale.getLanguage()) {`

return switch(exp) {

Requested in issue:
return Optional.ofNullable(switch (arg) {

Something is weird, I think we mixing switch as statement and like expression. In statement, spaces around might make sense, in expression form we should skip it completely for preceded and following spaces. So there should be two violations disappear https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/e6cdb69_2024152818/reports/diff/openjdk17/index.html#A4 .

If we agree on this we should just mention in doc that we don't handle switch expressions at all.

Thought?

@strkkk
Copy link
Member Author

strkkk commented May 7, 2024

@romani it makes sense imo. There is WhitespaceAfter check if user wants to force whitespace after switch.

@strkkk strkkk requested a review from nrmancuso May 8, 2024 01:29
@nrmancuso
Copy link
Member

If we agree on this we should just mention in doc that we don't handle switch expressions at all.

Yes, I am in agreement, @strkkk let's update the docs to reflect this.

@strkkk
Copy link
Member Author

strkkk commented May 10, 2024

ok, done. I added a note

@strkkk
Copy link
Member Author

strkkk commented May 10, 2024

fixed

@romani
Copy link
Member

romani commented May 10, 2024

3 more CI failures , restarted

@strkkk
Copy link
Member Author

strkkk commented May 10, 2024

@romani build is green

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.

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

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

@romani romani requested a review from rnveach May 10, 2024 21:40
@romani romani assigned rnveach and unassigned romani May 10, 2024
@rnveach rnveach merged commit 4f2a322 into checkstyle:master May 13, 2024
69 checks passed
@romani
Copy link
Member

romani commented May 14, 2024

@strkkk , if will be awesome if you fix your account in CircleCI, this CI is not running for your account. This created a leak.

@strkkk strkkk deleted the 14825_switch_expr branch May 14, 2024 12:57
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.

WhitespaceAround reports a violation when switch expression is passed as a method argument
4 participants