Add first draft of a rule description style guide

[pnq32]

Based on #4384, this PR contains a draft of a style guide for rule descriptions.

[BraisGabin]

Thank you so much!

I think that we should define the 3 different "descritpions" that a rule have:

• The on that is in the javadoc of the rule. That description is used in the html documentation that detetk have. An example: https://detekt.github.io/detekt/coroutines.html. For example, I think that this description should have links to different sources to explain why something is a bad pattern.
• The rule description (Inside the class Issue). That's the description of the issue that is used in the reports like html and sarif. In general I think that it should be small than the one in the javadoc. We canalways add a link to the documentation in the html or sarif report to get the complete description with samples, default configurations etc.
• The issue message. That's the message that tells you what's wrong with your code.

I think that there are points in this guide that are general for all of these descriptions but, in geenral, it is targeting the last one. What do you think about talking a bit about this three descriptions and be more precise about what to write in each description. For example I just saw that some issue messagges directly use the rule description. That doesn't feel right.

[pnq32]

Thank you very much for your detailed response. I have to admit that until now, I was not really aware of the internal structure that Detekt uses to distinguish rule documentation, issue descriptions, and code smell descriptions.

If I understand you correctly, you are referring to these three kinds of descriptions:

1. Javadoc comments such as this one. They are used to generate documentation sections such as this one.
2. Static String values used to populate the description parameter of an Issue, such as in this line. They are used as 'table headers' in reports (such as an HTML report) and serve as the 'container' for all code smells that the rule has identified. They are used neither from FindingsReport nor from LiteFindingsReport presented to the user via the console.
3. Dynamically generated String values that a rule uses to to label each identified code smell, such as here. In generated reports, they are presented to the user as 'items' belonging to their 'enclosing issue'. Furthermore, the new LiteFindingsReport makes use of these messages to generate console reports.

Is this correct or did I get something wrong here? If it is, then I fully agree with your suggestion.

What do you think of the following rough ideas?

1. Javadoc strings are focused on the strategy pursued by a rule and should answer questions such as the following:
   • What exactly does this rule check for?
   • How does it try to identify violations?
   • Why (according to which references) is it important to identify such violations?
   • What does the rule do to identify specific recommendations targeted at the developer?
2. Issue descriptions are formulated under the assumption that at least one code smell has been identified by the respective rule. They should give a concise description of the 'general violation type' that was found in the codebase and, if applicable, summarize why such a violation is bad practice. If it is not obvious, they should also give a rough summary of what is generally done to tackle such violations.
3. Code smell messages are concise extensions of issue messages. They have the goal to tell the developer exactly what the violation is, ideally using specific variable/function names or similar knowledge from the analyzed code. Furthermore, they can give much more specific recommendations (if it makes sense in the specific context).

For instance, assume that NonBooleanPropertyPrefixedWithIs is extended to handle additional prefixes (such as has or can) and therefore renamed to something like MisleadingBooleanPropertyPrefix:

1. The current Javadoc string would obviously need to be extended for the other prefixes and might benefit from a more detailed explanation of why this naming is bad practice. But in general, I think it is fine and can just remain as it is.
2. The issue description would look something like this: Non-boolean property with prefix suggesting boolean type. This might mislead clients of the API and facilitate programming errors on their side. Such prefixes should therefore be removed..
3. A code smell message would be more specific and focused on the actual violation: Non-boolean property `hasXyz` suggests a boolean type., potentially with a very short recommendation.

If it makes sense, I think it would be nice to encourage 'code-oriented' recommendations in code smell messages. The rule checking for magic numbers, for instance, could determine whether or not you are passing the magic number to a Java method or a Kotlin function. For the Java case, the message could look like this:

• Magic number `4` passed to `Class::method`. Consider introducing a named constant.

If it is a Kotlin function and the magic number is only used once, it could look like this:

• Magic number `4` passed to `abc`. Consider passing it as a named argument.

Please let me know what you think. If this makes sense, I am happy to update the current draft accordingly.

[codecov]

Codecov Report

Merging #4386 (562ede3) into main (e2cc197) will decrease coverage by 0.02%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #4386      +/-   ##
============================================
- Coverage     84.33%   84.31%   -0.03%     
- Complexity     3269     3284      +15     
============================================
  Files           473      474       +1     
  Lines         10343    10437      +94     
  Branches       1825     1861      +36     
============================================
+ Hits           8723     8800      +77     
- Misses          668      670       +2     
- Partials        952      967      +15     

Impacted Files	Coverage Δ
...rturbosch/detekt/rules/style/StyleGuideProvider.kt	100.00% <0.00%> (ø)
...urbosch/detekt/rules/style/ExpressionBodySyntax.kt	83.33% <0.00%> (ø)
...urbosch/detekt/rules/style/UselessCallOnNotNull.kt	89.13% <0.00%> (ø)
...arturbosch/detekt/rules/bugs/CastToNullableType.kt	86.66% <0.00%> (ø)
...osch/detekt/rules/style/OptionalAbstractKeyword.kt	89.47% <0.00%> (ø)
.../detekt/generator/collection/MultiRuleCollector.kt	74.62% <0.00%> (ø)
...rbosch/detekt/rules/bugs/ImplicitUnitReturnType.kt	87.50% <0.00%> (ø)
...osch/detekt/rules/bugs/UnsafeCallOnNullableType.kt	82.35% <0.00%> (ø)
...etekt/rules/style/FunctionOnlyReturningConstant.kt	95.45% <0.00%> (ø)
...etekt/rules/complexity/StringLiteralDuplication.kt	95.74% <0.00%> (ø)
... and 8 more

---

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 e2cc197...562ede3. Read the comment docs.

[BraisGabin]

Is this correct or did I get something wrong here?

100% correct. You explained way better than me.

Please let me know what you think. If this makes sense, I am happy to update the current draft accordingly.

I like it!

The only thing I'm not sure is if we should explain "How" the rule detects the issues in point 1. I think that we should focus on say what exactly does it detect and why it is important (with references if possible). I think that's the more important information that you need in the documentation. That's what you are going to read to decide if you want to enable/disable a rule. So I think that these two points shouldn't be there:

   * How does it try to identify violations?
   * What does the rule do to identify specific recommendations targeted at the developer?

Why do you think those two points are important?

[pnq32]

   * How does it try to identify violations?
   * What does the rule do to identify specific recommendations targeted at the developer?

Why do you think those two points are important?

I agree that these two points are much less important than the other two aspects. In fact, I think that they are irrelevant for 95 percent of rules. However, my initial idea was that for rather sophisticated rules (with a lot of configuration options), some knowledge about what the rule does (instead of what it is generally concerned with) would simplify the configuration process.

For example, think of an improved MagicNumber rule that allows you (as the person coming up with the config) to specify if Detekt should recommend using named arguments over introducing named constants if the magic number is not passed to a Java function. Then the Javadoc could go something like this:

This rule recommends to replace each magic number detected in a given Kotlin file with more suitable constructs. For each magic number passed to a Java method, it will issue a recommendation to replace it with a named constant. For magic numbers passed to Kotlin functions, the issued recommendation depends on the value of the the preferNamedArguments option. If it is active, the rule will recommend to pass the magic number using a named argument. Otherwise, it will recommend to either replace the magic number with a named constant or to pass it using a named argument.

Nevertheless, I totally get your point. Considering that developers will have the opportunity to just look at the code, it is probably neither necessary not worth the effort. So let's just focus on what the rule checks for and why that might be bad practice.

One last question before I get started: Do you happen to know how this relates to #4163? More specifically, what exactly is it that GitHub shows in the table: issue descriptions or code smell messages? I was thinking that it would probably make sense to encourage short texts for whatever is displayed in this overview.

[BraisGabin]

I'm not sure what is shown in each case but I think that in that report we are setting the rule description as the title. In detekt we don't have a title for a rule so probably we fail in that map. Probably we should use the id of the rule there instead of the description.

 was thinking that it would probably make sense to encourage short texts for whatever is displayed in this overview.

I think that the only description that should be long is the one in javadoc. The other two should be direct to the point. One telling what the rule checks or what an issue raised by this rule means. And the other saying what was found and (if possible) a recommendation about how to fix it.

[pnq32]

Alright, I have now updated the style guide to cover the three categories you mentioned.

Could you please check and see if this reflects what you had in mind?

[BraisGabin]

👏👏👏 Great work! 👏👏👏

I really like it :) I will keep this PR open for now to get more feedback but for me this is ready to merge.

The only thing is that, probably, we should add a line in this file https://github.com/detekt/detekt/blob/main/.github/CONTRIBUTING.md with a link to this guide.

[pnq32]

That is good to hear, thank you very much for your help!

One last remark from my side: I was unsure whether or not we want to encourage configuration-dependent issue descriptions. I have left it unspecified for now, but I think that it might actually be worthwhile to handle this consistently as well. For instance, think of the hypothetical MagicNumbers rule with a preferNamedArguments option again. Assume that the default issue description goes something like this:

Numeric literal used as magic number. To avoid confusion on the side of the client, introduce a named constant or pass such literals as a named argument.

If preferNamedArguments is activated, should the issue description change to something like this?

Numeric literal used as magic number. To avoid confusion on the side of the client, try to pass such literals as a named argument. If this is not possible, introduce a named constant.

Or should the issue description be an entirely static text?

[BraisGabin]

They could be dynamic but I don't know if that would be too specific for the guide. I mean, if someone wants to improve a rule adding dynamic description would be great but I don't know about encouraging it.

[BraisGabin]

@schalkms I know that you editted lots of rules descriptions based on this style guide so: could you review this? I would like to have multiple approvals here before merging.

[schalkms]

I read this guide and I have to say, I really like your writing style. It's excellent. You have done a great job! 👏
Thanks for spending the time to submit this PR and improve the documentation a lot! 🙏

[pnq32]

@schalkms: Thank you for trying to apply the style guide to the codebase and for your helpful feedback!

I think there is one aspect that we should clarify before finalizing it, though. The way the guidelines are currently formulated, they require you to describe the violation of an issue and allow you to (optionally) add a rationale and/or a recommendation. If you follow this style guide closely, the following issue description would actually be valid:

• ✔️: Public library class.

I agree that in this specific case, it would actually be beneficial to extend it a bit, but the current style guide does not require you to do so. After all, most detekt users would probably understand what they are expected to do. At the same time, the style guide does not allow you to omit the violation part. Therefore, strictly speaking, the following issue descriptions would not comply to the guide:

• ❌: Don't try to be smarter than the JVM. Your code should work independently whether the...
• ❌: The `next()` method of an Iterator implementation should throw a NoSuchElementException...
• ❌: Function invocations which have many parameters should be named.
• ❌: Library classes should not be public.

I obtained these examples from the changes you have just applied to existing issue descriptions. Combined with your comment above, I can see that you seem to have slightly different idea about how issue descriptions should look like. The set of rules I came up with seems to be both too lax and too strict at the same time. What do you think, is this the case? If it is: Could you maybe try to summarize what your idea of the ideal issue description is? I would then try to incorporate it into the draft.

[schalkms]

I think we have the same understanding regarding how an issue description should look like with the mandatory violation but optional recommendation part. As I have said, it's quite a tedious task to go over ~230 rules, so I might have missed a few descriptions. I'm happy to adapt and further improve the rule descriptions.

Regarding example 1 and 2, I think it's quite clear what the violation is, because it reads like this in the report, whereas example 3 and 4 should be adapted.

1. ❌: Don't try to be smarter than the JVM. Your code should work independently whether the...

ExplicitGarbageCollectionCall: Don't try to be smarter than the JVM. Your code should work independently whether the garbage collector is disabled or not. If you face memory issues, try tuning the JVM options instead of relying on code itself.

2. ❌: The `next()` method of an Iterator implementation should throw a NoSuchElementException...

IteratorNotThrowingNoSuchElementException: The next() method of an Iterator implementation should throw a NoSuchElementException when there are no more elements to return.

3. ❌: Function invocations which have many parameters should be named.
4. ❌: Library classes should not be public.

[pnq32]

Alright, I have now tried to address all your remarks. Most importantly, I have merged the style guide into CONTRIBUTING.md.

While doing so, I was unfortunately unable to make proper use of the <details> block. When I try to wrap portions of the style guide into these blocks, some Markdown formatting seems to break. For instance, inline code (wrapped by `) does not seem to work anymore and the vertical space between bullet point items seems off. If you know suitable workarounds, I would be happy if you could take a look at how to do this. For now, I have just used subheadings to structure the document.

What do you think, does this go into the direction you had in mind?

[schalkms]

I read through it and completely approve it. Good documentation is highly valued.
Sorry for the review and merging delay from our side, so thanks for being patient with us.

While doing so, I was unfortunately unable to make proper use of the

block.

Oh, I didn't know that. Using subheadings is also fine.

What do you think, does this go into the direction you had in mind?

👍

[BraisGabin]

Again, thank you so much @pnq32 :)

[pnq32]

Sorry for the review and merging delay from our side, so thanks for being patient with us.

Don't worry, I did not perceive the iterations as a delay. After all, there were a few decisions to make.

Thank you very much for your help to both of you! 🙂