Rename IsPropertyNaming to NonBooleanPropertyPrefixedWithIs?

[arturbosch]

I'm not sure if this is a better name but I wanted to start a discussion if we should rename the rule.

Imo IsPropertyNaming could lead to confusion.

[codecov]

Codecov Report

Merging #2819 into master will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #2819      +/-   ##
============================================
+ Coverage     79.78%   79.81%   +0.03%     
- Complexity     2506     2507       +1     
============================================
  Files           427      427              
  Lines          7534     7537       +3     
  Branches       1420     1420              
============================================
+ Hits           6011     6016       +5     
+ Misses          767      766       -1     
+ Partials        756      755       -1     

Impacted Files	Coverage Δ	Complexity Δ
...tlab/arturbosch/detekt/rules/naming/NamingRules.kt	93.33% <100.00%> (+0.35%)	18.00 <0.00> (ø)
...t/rules/naming/NonBooleanPropertyPrefixedWithIs.kt	85.18% <100.00%> (ø)	12.00 <1.00> (?)

---

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 2dd828c...edbb68f. Read the comment docs.

[cortinico]

Imho BooleanPropertyNaming is a better name. I agree that Is in the first part of the name is not ideal as it's also a verb and it doesn't help to immediately understand the meaning of the rule.

If possible I'd try to keep the *Naming suffix as we already have several rules that follows this convention:

./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ClassNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/IsPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/EnumNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ObjectPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/TopLevelPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/PackageNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionParameterNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/VariableNaming.kt

[arturbosch]

Imho BooleanPropertyNaming is a better name. I agree that Is in the first part of the name is not ideal as it's also a verb and it doesn't help to immediately understand the meaning of the rule.

If possible I'd try to keep the *Naming suffix as we already have several rules that follows this convention:

./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ClassNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/IsPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/EnumNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ObjectPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/TopLevelPropertyNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/PackageNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/ConstructorParameterNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/FunctionParameterNaming.kt
./src/main/kotlin/io/gitlab/arturbosch/detekt/rules/naming/VariableNaming.kt

But this rule will report the inverse of BooleanPropertyNameing -> all properties which are not of type boolean and start with an is are forbidden.

[BraisGabin]

I like your proposal more that the current one. But the problem with the name make me think... Should this be part of the PropertyNaming as a flag? I thought about this after reading your description in the last comment.

[cortinico]

Should this be part of the PropertyNaming as a flag? I thought about this after reading your description in the last comment.

Piggybacking on this. Imho we should have a BooleanPropertyNaming that enforces is- named boolean property (not active by default as it's really strict), but that has a flag also to verify non boolean properties that are wrongly named (basically what IsPropertyNaming is doing now).

[BraisGabin]

A lot of people look at detekt as a guideline. If we add something like that they will think that name all your boolean properties with is is a good thing, and I don't think so. I mean, I can understand that a team take that convention but as a normal basis I don't think so.

[cortinico]

A lot of people look at detekt as a guideline

yup, agree on this, it's probably too strict and will lead to a lot of local @Suppress if activated.

[schalkms]

I agree with @BraisGabin here. Detekt should have a controversial ruleset, which is disabled by default. This rule would be a perfect candidate for this new ruleset.

[BraisGabin]

🤔 I think that we mixed some ideas here. I think that this rule is good. Naming a property isPrivate and define it as Int is bad. Because if I read is I will think that this is a boolean. It's the same when a class is called val animal = dog() you directly think that dog is a function but it could be actually a class.

I mean, the rule, as it is right now is not controversial.

If we force that ALL the boolean variables are prefixed with is that's controversial for sure. And I never saw any article encoraging that notation.

My point is: should we merge this rule with VariableNaming? I know, right now VariableNaming is just a regex. But I think that the best fit for IsPropertyNaming rule is to merge it in VariableNaming instead of raneming it.

[cortinico]

But I think that the best fit for IsPropertyNaming rule is to merge it in VariableNaming instead of raneming it.

Yup that's probably the best place to go.
The side effect of this is that VariableNaming is not relying on type and symbols resolution, while the current IsPropertyNaming is.

So we will end up with a rule that has a flag that enables a more "advanced" naming analysis only if a BindingContext is provided. I don't know if we have already similar rules that offer "mixed" analysis and how we want to proceed on this front.

[arturbosch]

Ok, I will leave this PR open for now, remove IsPropertyNaming for 1.10.0 from the ruleset, release 1.10 and refactor the rule to a property for VariableNaming for 1.11 or 1.10.1.

Edit: I'm not sure if a configuration property like reportNonBooleansStartingWithIs would not confuse users more.
Maybe we should leave this as a separate rule?

[cortinico]

Maybe we should leave this as a separate rule?

How do we feel about the controversial ruleset suggested by @schalkms:

Detekt should have a controversial ruleset, which is disabled by default. This rule would be a perfect candidate for this new ruleset.

I also feel this rule could probably be moved there

[arturbosch]

@cortinico @BraisGabin @schalkms
Hm, having a new rule BooleanPropertyNaming as @cortinico mentioned should be controversal.
But should this one be controversal? I just reread the discussion and feel that this rule with the new name NonBooleanPropertyPrefixedWithIs could just be under the style rule set?
Maybe we should postpone introducing a new rule set until 2.0 ?

Edit: well naming is style, so lets leave it there?

[cortinico]

But should this one be controversal? I just reread the discussion and feel that this rule with the new name NonBooleanPropertyPrefixedWithIs could just be under the style rule set?
Maybe we should postpone introducing a new rule set until 2.0 ?

Agree here. For what concerns this specific rule, I'd +1 the new name NonBooleanPropertyPrefixedWithIs. The convo around BooleanPropertyNaming can be resumed once we land 2.x

Edit: well naming is style, so lets leave it there?

naming and style are actually two different ruleset right? This should definitely live in the naming one.