-
-
Notifications
You must be signed in to change notification settings - Fork 758
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 first draft of a rule description style guide #4386
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
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:
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?
For instance, assume that
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:
If it is a Kotlin function and the magic number is only used once, it could look like this:
Please let me know what you think. If this makes sense, I am happy to update the current draft accordingly. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
100% correct. You explained way better than me.
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:
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
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. |
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.
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. |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏👏👏 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.
Co-authored-by: Brais Gabín <braisgabin@gmail.com>
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
If
Or should the issue description be an entirely static text? |
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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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! 🙏
Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>
@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:
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:
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. |
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.
|
Alright, I have now tried to address all your remarks. Most importantly, I have merged the style guide into While doing so, I was unfortunately unable to make proper use of the What do you think, does this go into the direction you had in mind? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
👍
Again, thank you so much @pnq32 :) |
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! 🙂 |
Based on #4384, this PR contains a draft of a style guide for rule descriptions.