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.

Sign up for GitHub

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

Jump to bottom

MandatoryBracesIfStatement to highlight only if/else keywords #5317

Closed
TWiStErRob opened this issue Sep 19, 2022 · 2 comments · Fixed by #5407
Closed

MandatoryBracesIfStatement to highlight only if/else keywords #5317

TWiStErRob opened this issue Sep 19, 2022 · 2 comments · Fixed by #5407
Labels
feature good first issue Issue that is easy to pickup for people that are new to the project help wanted

Comments

@TWiStErRob
Copy link
Member

TWiStErRob commented Sep 19, 2022 •
edited

Pretty much the same as #5316, but a different rule.

Context

image

Warnings exists in some codebases, and that's ok, but then they make the code harder to read, then it becomes a problem. A warnings should highlight where the problem is, specifically, but some rules report on the blocks which could be quite large, therefor hiding potential crashes, which could be quite serious. In the example above the MandatoryBracesIfStatement hides an UnsafeCallOnNullableType (potential NPE).

Expected Behavior of the rule

Move the highlight of the reported violation to the if keyword and the else keyword. Note to implementer: mind else ifs.
@BraisGabin BraisGabin added help wanted good first issue Issue that is easy to pickup for people that are new to the project labels Sep 20, 2022
dizney added a commit to dizney/detekt that referenced this issue Oct 12, 2022
@dizney
Report if/else as issue location instead of block
3e9d812 
Report mandatory braces issues on the actual if and/or else keywords
instead of the then or else code blocks to prevent hiding potential
other issues in those code blocks.

Closes detekt#5317
@dizney dizney mentioned this issue Oct 12, 2022
Merged
@dizney
Copy link
Contributor

dizney commented Oct 13, 2022

I have created a PR (#5407) to change the behavior of the rule.

The hightlighting looks like this with the PR change:

Screenshot 2022-10-13 at 15 04 04

BraisGabin pushed a commit that referenced this issue Oct 13, 2022
@dizney
Report if/else as issue location instead of block (#5407)
48f5615 
Report mandatory braces issues on the actual if and/or else keywords
instead of the then or else code blocks to prevent hiding potential
other issues in those code blocks.

Closes #5317
@TWiStErRob
Copy link
Member Author

Oh, yeah! Amazing @dizney, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature good first issue Issue that is easy to pickup for people that are new to the project help wanted
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Report if/else as issue location instead of block dizney/detekt
3 participants
@BraisGabin @dizney @TWiStErRob