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

Add ignoreOverridden support for BooleanPropertyNaming rule #4654

Conversation

mhernand40
Copy link
Contributor

This PR addresses #4652.

@mhernand40 mhernand40 force-pushed the add-ignore-overridden-support-for-boolean-property-naming-rule branch from c794465 to 25b1ddb Compare March 30, 2022 00:16
@mhernand40 mhernand40 force-pushed the add-ignore-overridden-support-for-boolean-property-naming-rule branch from 25b1ddb to e7a5566 Compare March 30, 2022 00:18
@cortinico
Copy link
Member

Great stuff 👍

@cortinico cortinico added this to the 1.20.0 milestone Mar 30, 2022
@@ -35,6 +36,9 @@ class BooleanPropertyNaming(config: Config = Config.empty) : Rule(config) {
@Configuration("naming pattern")
private val allowedPattern: Regex by config("^(is|has|are)", String::toRegex)

@Configuration("ignores properties that have the override modifier")
private val ignoreOverridden: Boolean by config(false)
Copy link
Member

Choose a reason for hiding this comment

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

I would default this to true. I know, this is to avoid behaviour change but for me this is more a bug fix than a "behaviour change".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be happy to address and make that change. Would others care to weigh in what their thoughts are before I do so? Thank you 🙂

Copy link
Member

Choose a reason for hiding this comment

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

Agree, this is reasonable to be defaulted to true. Is a "bug-fix on steroids"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thank you so much. 🙂

@BraisGabin
Copy link
Member

Thanks for the PR it looks great :)

@BraisGabin BraisGabin merged commit abd3851 into detekt:main Mar 31, 2022
chao2zhang pushed a commit that referenced this pull request Apr 8, 2022
* Add ignoreOverridden support for BooleanPropertyNaming

* Default to true for ignoreOverridden

* Fix tests

* Update default-detekt-config.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants