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

New Rule: BracesOnWhenStatements with configurable parameters #5838

Merged
merged 12 commits into from Apr 20, 2023

Conversation

VitalyVPinchuk
Copy link
Contributor

For #5133

Similar to #5700

@github-actions github-actions bot added the rules label Feb 26, 2023
@github-actions
Copy link

github-actions bot commented Feb 26, 2023

Messages
📖 Thanks for adding a new rule to detekt ❤️
We detekted that you added tests, to your rule, that's awesome!

We detekted that you updated the default-detekt-config.yml file, that's awesome!

Generated by 🚫 dangerJS against 70183de

@VitalyVPinchuk VitalyVPinchuk changed the title New Rule: BracesOnWhenStatements with configurable parameters New Rule: BracesOnWhenStatements with configurable parameters Feb 26, 2023
@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Merging #5838 (0e45543) into main (d067a13) will increase coverage by 0.09%.
The diff coverage is 93.18%.

❗ Current head 0e45543 differs from pull request most recent head 70183de. Consider uploading reports for the commit 70183de to get more accurate results

@@             Coverage Diff              @@
##               main    #5838      +/-   ##
============================================
+ Coverage     84.59%   84.68%   +0.09%     
- Complexity     3776     3859      +83     
============================================
  Files           546      550       +4     
  Lines         12916    13109     +193     
  Branches       2268     2313      +45     
============================================
+ Hits          10926    11102     +176     
- Misses          861      868       +7     
- Partials       1129     1139      +10     
Impacted Files Coverage Δ
...bosch/detekt/rules/style/BracesOnWhenStatements.kt 93.02% <93.02%> (ø)
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <100.00%> (ø)

... and 59 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

The tests are so easy to read!! 😍. I left some comments but this PR looks really really good!

@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Mar 3, 2023
@cortinico cortinico added this to the 1.23.0 milestone Mar 3, 2023
Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

Nice one, a few changes are needed to make sense of consistent/multi-statement. But the majority is good! Thank you!

…/rules/style/BracesOnWhenStatements.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
…/rules/style/BracesOnWhenStatements.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
…/rules/style/BracesOnWhenStatements.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
…/rules/style/BracesOnWhenStatements.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
@VitalyVPinchuk
Copy link
Contributor Author

Also, I can't engage this rule in detekt.yml as it fails regardless of any parameter.

@BraisGabin
Copy link
Member

Also, I can't engage this rule in detekt.yml as it fails regardless of any parameter.

I don't understand the your statment. Can you elavorate?

@VitalyVPinchuk
Copy link
Contributor Author

Also, I can't engage this rule in detekt.yml as it fails regardless of any parameter.

I don't understand the your statment. Can you elavorate?

I mean if I turn on this rule, detekt task fails. No matter what parameters I use in the rule: consistent, necessary, etc. We have to fix codebase a bit

@BraisGabin
Copy link
Member

Oh, sure. No problem about that. We can enable it later in other pr with the proper fixes.

Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, move was longer than expected. This looks good now, just some final touches / simplifications. Happy to help finishing up, find me on Slack. Please look through all the unresolved conversations on the PR, there might be some hiding behind expanders.

Copy link
Member

@TWiStErRob TWiStErRob left a comment

Choose a reason for hiding this comment

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

One last round 🏆 .
Please have a look at all (potentially hidden) unresolved conversations.

…/rules/style/BracesOnWhenStatements.kt

Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
@BraisGabin BraisGabin merged commit 70161db into detekt:main Apr 20, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants