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

Separating ComplexMethod rule into CyclomaticComplexMethod and CognitiveComplexMethod #5442

Merged
merged 10 commits into from
Oct 23, 2022
Merged

Separating ComplexMethod rule into CyclomaticComplexMethod and CognitiveComplexMethod #5442

merged 10 commits into from
Oct 23, 2022

Conversation

sanggggg
Copy link
Contributor

Fixes #5349

@github-actions github-actions bot added the rules label Oct 19, 2022
@github-actions
Copy link

github-actions bot commented Oct 19, 2022 •
edited
Loading

Messages
📖 Thanks for adding a new rule to Detekt ❤️

Generated by 🚫 dangerJS against 2a0ae60

@sanggggg sanggggg mentioned this pull request Oct 19, 2022
Closed
@BraisGabin
Copy link
Member

Did you create the PR to get early reviews or to get early CI checks? Or both? In other words, even the PR is a Draft, do you want me to review it or do you want to work a bir more and make CI happy first?

@sanggggg sanggggg marked this pull request as ready for review October 19, 2022 14:09
@sanggggg
Copy link
Contributor Author

@BraisGabin Thank you for checking, I just opened it to get a review including your feedback.
(I opened it to receive an early review of the changes!)

BraisGabin
BraisGabin reviewed Oct 19, 2022
View reviewed changes
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.

Good job! Just two minor things and you need to make CI happy. I think that fixint the issues that I comment it will be happy but I'm not 100% sure because I don't fully understand why it is complaining.

...exity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/CognitiveComplexMethod.kt Outdated
* - __Nesting Level Increments__ - `if`, `when`, `for`, `while`, `do while`, `catch`, `nested function`
* - __Additional Complexity Increments by Nesting Level__ - `if`, `when`, `for`, `while`, `do while`, `catch`
*/
@ActiveByDefault(since = "1.22.0")
Copy link
Member

Choose a reason for hiding this comment

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

You should remove this line. And probably run ./gradlew generateDocumentation because CI is complaining that the documentation is not on sync (probably detekt-core/src/main/resources/default-detekt-config.yml or detekt-core/src/main/resources/deprecation.properties)

That couls also fix the othere CI issues... I don't understand why this rule is executed...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing ActiveByDefault & sync config works fine 😄
2a0ae60

...xity/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/complexity/CyclomaticComplexMethod.kt Outdated Show resolved Hide resolved
sanggggg and others added 2 commits October 20, 2022 10:03
@sanggggg @BraisGabin
Update detekt-rules-complexity/src/main/kotlin/io/gitlab/arturbosch/d…
236d3ce 
…etekt/rules/complexity/CyclomaticComplexMethod.kt

Co-authored-by: Brais Gabín <braisgabin@gmail.com>
@sanggggg
fix: remove conflicting activeByDefault and sync
2a0ae60
@sanggggg sanggggg requested a review from BraisGabin October 20, 2022 01:36
BraisGabin
BraisGabin approved these changes Oct 20, 2022
View reviewed changes
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.

LGTM

@BraisGabin BraisGabin added this to the 1.22.0 milestone Oct 20, 2022
@BraisGabin
Copy link
Member

I just added the 1.22 milestone to this one. But I would know what others think about that. We are at RC2. Should we wait until the stable version to merge it?

@cortinico
Copy link
Member

cortinico commented Oct 21, 2022 •
edited
Loading

I just added the 1.22 milestone to this one. But I would know what others think about that. We are at RC2. Should we wait until the stable version to merge it?

I think we might have a RC3 as we received several regressions in RC3 so it's probably also fine to merge this.

@BraisGabin BraisGabin merged commit 7b632b2 into detekt:main Oct 23, 2022
@BraisGabin
Copy link
Member

I fully agree with a RC3. The number of contributions this month is great :) I imagine that hacktoberfest is working as a charm :)

antonis pushed a commit to wordpress-mobile/WordPress-Android that referenced this pull request Jul 18, 2023
Adds ignore for SimpleWhenEntries in CyclomaticComplexMethod now caug…
2ccba23 
…ht on detekt detekt/detekt#5442
antonis pushed a commit to wordpress-mobile/WordPress-Android that referenced this pull request Jul 18, 2023
Adds ignore for suppresses for CyclomaticComplexMethod specific cases…
c9e2b81 
… now caught on detekt with detekt/detekt#5442
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BraisGabin BraisGabin BraisGabin approved these changes

Assignees
No one assigned
Labels
rules
Projects
None yet
Milestone
1.22.0
Development

Successfully merging this pull request may close these issues.

Allow the user to choose between cyclomatic and cognitive complexity in ComplexMethod rule
3 participants
@sanggggg @BraisGabin @cortinico