New rule: BooleanPropertyNaming

[AlinaRakhimova]

I added a new rule to check the naming of boolean variables to improve the readability of the code.

For example,

val enabled = true

Instead of should prefer to

val isEnabled = true

[schalkms]

Hello Alina,
I don't know if you are aware of it, but there exists already a rule, which works essentially the same as your rule. The controversial rule is called NonBooleanPropertyPrefixedWithIs. Hence, detekt disables this rule by default.

I don't think detekt needs a second rule even if minor differences exist. Therefore, I have to unfortunately declare this as won't merge. Still, you can use this rule in a custom rule set as you like. See here for more details.

https://github.com/detekt/detekt/blob/main/detekt-rules-naming/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/NonBooleanPropertyPrefixedWithIs.kt

[shumidub]

But, this is different rules. NonBooleanPropertyPrefixedWithIs - for NOT boolean property and don't check boolean properties naming.

BooleanPropertyPrefixedWithAllowedWords - for booleans.

[cortinico]

Thanks for taking the time to add a new rule 🙏

I believe we could consider this rule, but it falls a bit in the controversial area.
Specifically I don't agree that:

val enabled = true

is less readable than:

val isEnabled = true

but that's debatable, so I can understand how developers might want to enforce some prefixes on their Boolean property.

I would like to get other maintainers opinion on this.

Some things that needs to be addressed/discussed:

• The rule name should probably be tweaked. Ideally BooleanPropertyPrefix or BooleanPropertyNaming.
• We already have a similar rule that does this, is VariableNaming. Your rule is basically doing the same behavior but just for Boolean property. I'm unsure if this should be a separate rule or we should extend VariableNaming (that will impose type resolution on this rule, making it a "mixed rule" that we try to avoid Rules with mixed behavior based on Type resolution #2994).
• Do we have some reference from the official Kotlin guidelines where the boolean variable naming is suggested? If so, that should be linked in the documentation.

[codecov]

Codecov Report

Merging #3795 (85c0caa) into main (c356897) will increase coverage by 0.02%.
The diff coverage is 90.90%.

❗ Current head 85c0caa differs from pull request most recent head 0418e30. Consider uploading reports for the commit 0418e30 to get more accurate results

@@             Coverage Diff              @@
##               main    #3795      +/-   ##
============================================
+ Coverage     83.53%   83.55%   +0.02%     
- Complexity     3165     3179      +14     
============================================
  Files           458      459       +1     
  Lines          9053     9085      +32     
  Branches       1761     1765       +4     
============================================
+ Hits           7562     7591      +29     
  Misses          564      564              
- Partials        927      930       +3     

Impacted Files	Coverage Δ
...bosch/detekt/rules/naming/BooleanPropertyNaming.kt	89.65% <89.65%> (ø)
...tlab/arturbosch/detekt/rules/naming/NamingRules.kt	93.65% <100.00%> (+0.31%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c356897...0418e30. Read the comment docs.

[schalkms]

I believe we could consider this rule, but it falls a bit in the controversial area

The question is whether more controversial rules shall exist in detekt considering the increasing maintenance effort in this project.

We already have a similar rule that does this, is VariableNaming. Your rule is basically doing the same behavior but just for Boolean property. I'm unsure if this should be a separate rule or we should extend VariableNaming (that will impose type resolution on this rule, making it a "mixed rule" that we try to avoid #2994).

I agree with this and forgot to also reference the VariableNaming rule.

Do we have some reference from the official Kotlin guidelines where the boolean variable naming is suggested? If so, that should be linked in the documentation.

I don't think there exists one. This constitutes more of a common rule in many projects.

[BraisGabin]

Do we have some reference from the official Kotlin guidelines where the boolean variable naming is suggested? If so, that should be linked in the documentation.

I would not add this rule if there is not a source or reference to back it. I think that, as detekt, we should not be that opinated. #3150 was closed even with references. I understand the rule and maybe I would use it in my project too, but I don't think we should have it in our default rulesets.

[AlinaRakhimova]

Do we have some reference from the official Kotlin guidelines where the boolean variable naming is suggested? If so, that should be linked in the documentation.

I would not add this rule if there is not a source or reference to back it. I think that, as detekt, we should not be that opinated. #3150 was closed even with references. I understand the rule and maybe I would use it in my project too, but I don't think we should have it in our default rulesets.

If the user does not want to use this rule, he will not enable it in the config file

[shumidub]

The rule is very useful, it improves the readability and clarity of the code. It will be great if it will be available as an optional rule.

[chao2zhang]

With regards to references:

1. Java Bean requires boolean property to be prefixed with is (https://www.oracle.com/java/technologies/javase/javabeans-spec.html)
   In addition, for boolean properties, we allow a getter method to match the pattern:
   public boolean is<PropertyName>();
   This “is” method may be provided instead of a “get” method, or it may be provided in addition to a “get” method.
   In either case, if the “is” method is present for a boolean property then we will
   use the “is” method to read the property value.
   An example boolean property might be:

   public boolean isMarsupial();
   public void setMarsupial(boolean m);

2. Kotlin-Java interop (https://kotlinlang.org/docs/java-interop.html#getters-and-setters)
   Boolean accessor methods (where the name of the getter starts with is and the name of the setter starts with set) are represented as properties which have the same name as the getter method.

3. Android lint implemented checks requiring specific bean style for Java methods Accessor methods require a ‘get’ prefix or for boolean-returning methods an ‘is’ prefix can be used.
   https://cs.android.com/android-studio/platform/tools/base/+/mirror-goog-studio-master-dev:lint/libs/lint-checks/src/main/java/com/android/tools/lint/checks/InteroperabilityDetector.kt;l=141?q=Interoperability%20lint

The above references apply to Java code, yet we do not have a Kotlin coding convention encouraging or discouraging the naming pattern for a boolean property. My vote would be yes as this rule is rather easy to maintain and we could document these references in the KDoc so that we keep this rule disabled in the future.

[chao2zhang]

Other thoughts? We probably want to reach a consensus on this PR before the next release.

[cortinico]

I believe we could add this rule and have it disabled 👍 It's not as controversial as other and the maintenance cost is not as high.

[AlinaRakhimova]

Please use the annotation based rule configurations instead of kdoc.

I made it

[cortinico]

Is there anything missing to merge this?

[AlinaRakhimova]

Is there anything missing to merge this?

Hi! All discussions were resolved

[cortinico]

Hi! All discussions were resolved

Can I ask you to address the CI failures?

[AlinaRakhimova]

Hi! All discussions were resolved

Can I ask you to address the CI failures?

I'll try to do it

[marschwar]

I think the code itself is fine (once it compiles again 😃 - The config delegate was moved to a different package.).

I have an issue with its inconsistent documentation. I know that it started out to focus on variable name prefixes, but when the rule was renamed to BooleanPropertyNaming all mentions of "prefix" should have been removed from it as well. The users will not look at the code but rather its documentation so please be precise with what the rule does and how it can be configured.

Thank you for your contribution. I think it's really close. Sorry for the delay.

[cortinico]

Thanks for the contrib @AlinaRakhimova, sorry it took a bit longer than usual 🙏