Skip to content

Implement AlsoCouldBeApply Rule#5333

Merged
cortinico merged 10 commits into
detekt:mainfrom
Tommyten:misusedalso
Oct 4, 2022
Merged

Implement AlsoCouldBeApply Rule#5333
cortinico merged 10 commits into
detekt:mainfrom
Tommyten:misusedalso

Conversation

@Tommyten

Copy link
Copy Markdown
Contributor

This pullrequest adds a rule to detect also blocks, which could be simplified by using an apply block, as discussed in #518.

Closes #518

@github-actions github-actions Bot added the rules label Sep 22, 2022
@github-actions

github-actions Bot commented Sep 22, 2022

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

Generated by 🚫 dangerJS against 308e003

@BraisGabin BraisGabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks to implement this rule! 5 years since it was proposed 😄. LGTM.

@3flex

3flex commented Sep 24, 2022

Copy link
Copy Markdown
Member

I think the rule name should be a little more specific, like AlsoCouldBeApply. I haven't reviewed, but thank you for the PR, this is a great contribution!

@TWiStErRob TWiStErRob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks like a nice rule, well done!

Comment thread detekt-core/src/main/resources/default-detekt-config.yml Outdated
@cortinico cortinico added this to the 1.22.0 milestone Sep 25, 2022

@TWiStErRob TWiStErRob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm good, except pending conversations, @BraisGabin can you please look again?

@Tommyten Tommyten changed the title Implement MisusedAlso Rule Implement AlsoCouldBeApply Rule Sep 27, 2022

@TWiStErRob TWiStErRob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I had a dream about this, left 3 more comments :)

@BraisGabin BraisGabin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I re-reviewed the PR and it looks great. Thanks for the reviews @TWiStErRob and the changes @Tommyten.

@cortinico cortinico left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great stuff! Just a minor nit

@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Oct 4, 2022
@cortinico cortinico enabled auto-merge (squash) October 4, 2022 13:24
@cortinico cortinico merged commit 24dce51 into detekt:main Oct 4, 2022
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.

Rule: detect 'also' library function misuse instead of 'apply'

5 participants