From 779f605e33592c9e167bcaebadf1b4733232eeab Mon Sep 17 00:00:00 2001 From: pnq32 <96373665+pnq32@users.noreply.github.com> Date: Thu, 30 Dec 2021 20:12:18 +0100 Subject: [PATCH] Add first draft of a rule description style guide (#4386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add first draft of a rule description style guide * Fix a typo in the style guide draft Co-authored-by: Brais Gabín * Update style guide to cover the three types of descriptions * Discourage file names in code smell messages Co-authored-by: Brais Gabín * Fix two minor mistakes in the style guide * Explain the example on usage-aware recommendations * Reference the guide on authoing rule descriptions * Write detekt in lowercase * Add an empty line after every heading Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com> * Clean up the contribution guide * Incorporate latest review comments into the style guide * Fix a typo in the style guide * Use consistent line breaks in the style guide * Merge the style guide into the contribution guide * Delete the standalone style guide * Remove redundant space Co-authored-by: Brais Gabín Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com> --- .github/CONTRIBUTING.md | 306 +++++++++++++++++++++++++++++++++------- 1 file changed, 256 insertions(+), 50 deletions(-) diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 2bca3cf03ba..7f87a6512c2 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -1,38 +1,133 @@ # Contributing to detekt -- Read [this article](https://chris.beams.io/posts/git-commit/) before writing commit messages -- Use `gradle build -x dokkaJekyll` to build the source but exclude documentation jar generating to save time. -- `gradle detekt` should not report any errors -- This repository follows the [Kotlin Coding Conventions](https://kotlinlang.org/docs/reference/coding-conventions.html) which are enforced by ktlint when running `gradle detekt`. -- Make sure your IDE uses [ktlint](https://github.com/pinterest/ktlint) formatting rules as well as the settings in [.editorconfig](../.editorconfig) -- We use [Spek](https://github.com/spekframework/spek) for testing. Please use the `Spec.kt`-Suffix. For easier testing you might want to use the [Spek IntelliJ Plugin](https://plugins.jetbrains.com/plugin/10915-spek-framework). +- Read [this article](https://chris.beams.io/posts/git-commit/) before writing commit messages. +- Use `gradle build -x dokkaJekyll` to build the source but exclude documentation JAR generation to save time. +- Make sure that `gradle detekt` does not report any errors. +- This repository follows the [Kotlin coding conventions](https://kotlinlang.org/docs/reference/coding-conventions.html), + which are enforced by ktlint when running `gradle detekt`. +- Make sure your IDE uses [ktlint](https://github.com/pinterest/ktlint) formatting rules as well + as the settings in [.editorconfig](../.editorconfig). +- We use [Spek](https://github.com/spekframework/spek) for testing. Please use the `Spec.kt`-Suffix. + For easier testing you might want to use the [Spek IntelliJ Plugin](https://plugins.jetbrains.com/plugin/10915-spek-framework). - Feel free to add your name to the contributors list at the end of the readme file when opening a pull request. - The code in `detekt-api` and any rule in `detekt-rules` must be documented. We generate documentation for our website based on these modules. - If some Kotlin code in `resources` folder (like `detekt-formatting`) shows a compilation error, right click on it and use `Mark as plain text`. -### When implementing new rules ... +## Implementing new rules -- ... do not forget to add the new rule to a `RuleSetProvider` (e.g. StyleGuideProvider) -- ... do not forget to write a description for the issue of the new rule. -- ... add the [correct KDoc](#contents-and-structure-of-a-rules-kdoc) and [annotations](#rule-annotations) to your `Rule` class. This is used to generate documentation pages and the `default-detekt-config.yml` automatically. -- ... do not forget to test the new rule and/or add tests for any changes made to a rule. Run detekt on itself and other - kotlin projects with the `--run-rule RuleSet:RuleId` option to test your rule in isolation. Make use of +When implementing a new rule, do not forget to perform the following steps: +- Add the new rule to a `RuleSetProvider` (such as `StyleGuideProvider`). +- Test the new rule and/or add tests for any changes made to a rule. Run detekt on itself and other + Kotlin projects with the `--run-rule RuleSet:RuleId` option to test your rule in isolation. Make use of the `scripts/get_analysis_projects.kts` script to automatically establish a set of analysis projects. -- ... run `./gradlew generateDocumentation` to add your rule and its config options to the `default-detekt-config.yml`. -- ... do not forget to run `./gradlew build`. This will execute tests locally. -- To print the AST of sources you can pass the `--print-ast` flag to the CLI which will print each -Kotlin files AST. This can be helpful when implementing and debugging rules. -- To view the AST (PSI) of your source code you can use the [PSI Viewer plugin](https://plugins.jetbrains.com/plugin/227-psiviewer) for IntelliJ. -- be aware that your PR will stay open for at least two days so that other users can give feedback. +- Run `./gradlew generateDocumentation` to add your rule and its config options to the `default-detekt-config.yml`. +- Run `./gradlew build`. This will execute tests locally. -After some time and testing there is a chance this rule will become active on default. +The following remarks might help you during the implementation: +- To print the AST of sources, you can pass the `--print-ast` flag to the CLI. This will print each + Kotlin files AST. This can be helpful when implementing and debugging rules. +- To view the AST (PSI) of your source code, you can use the [PSI Viewer plugin](https://plugins.jetbrains.com/plugin/227-psiviewer) for IntelliJ. -#### Contents and structure of a rule's KDoc +The general policy regarding contributed rules is as follows: +- PRs will stay open for at least two days so that other users can give feedback. +- After some time and testing, there is a chance the contributed rule will become active by default. + +In order for your PR to get accepted, it is important that you add +suitable **annotations** and provide all required types of **descriptions**. +The following two subsections describe how to +properly annotate a rule and how to compose the different types +of descriptoins, respectively. + +### Rule annotations + +```kotlin +@ActiveByDefault(since = "1.0.0") +@RequiresTypeResolution +class SomeRule(config: Config = Config.empty) : Rule(config) { + + @Configuration("This is the description for the configuration parameter below.") + private val name: String by config(default = "whatever should be the default") + +} +``` + +Use the `@Configuration` annotation in combination with the `config` delegate to create +a configurable property for your rule. The name of the property will become the key and +the provided default will be the value in the `default-detekt-config.yml`. All information +are also used to generate the rule documentation in the wiki. +Note that a property that is marked with `@Configuration` must use the config +delegate (and vice versa). + +Rules annotated with `@ActiveByDefault` will be marked as active in the `default-detekt-config.yml`. +Generally, this will not be the case for new rules. + +A rule that requires type resolution must be marked with `@RequiresTypeResolution`. +See [the type resolution wiki page](../docs/pages/gettingstarted/type-resolution.md) for +more detail on this topic. + +The rule defined above will translate to a rule entry in the `default-detekt-config.yml`: +```yml +SomeRule: + active: true + name: 'whatever should be the default' +``` + +### Rule descriptions + +A rule incorporated into the detekt framework is generally accompanied by three +types of descriptions: +1. **Documentation string**: Using conventional + Javadoc/KDoc syntax, the documentation string is associated with the `Rule` + subclass implementing the considered rule. Documentation strings associated + with built-in rules are automatically pulled from the detekt codebase and + used to generate the rule set overview available on the + [detekt website](https://detekt.github.io/detekt). +2. **Issue description**: The issue description gives a summary of the code + smells that the respective rule is supposed to detect. From an implementation + point of view, it is the string that `Rule` subclasses pass to the + `description` parameter of the [`Issue` class][1]. In generated reports, the + issue description is often used to introduce a list of code smells that the + respective rule has identified. +3. **Code smell messages**: A code smell message is issued for every code + smell (or *violation*) identified within the codebase. Implementation-wise, + such a message is dynamically created during the construction of a + [`CodeSmell` instance][2]. In generated reports, these messages are listed + underneath the issue description. Built-in console reports such as + `LiteFindingsReport` use these messages to display identified code smells to + the user. + +It is important that you provide a documentation string, an issue description, +and a message for each code smell that the new rule generates. When authoring +these descriptions, you should keep two different target audiences in mind: +1. The **documentation string** is generally read by individuals who want to + learn about the *rule itself*. They might need to compose a detekt + configuration for their codebase, need to understand what the rule is + currently checking for in order to extend it, or are just interested in the + available detekt rule sets. +2. The **issue description** as well as **code smell messages** are presented to + developers whose codebase violates one or more rules that detekt checked for. + This audience is generally less interested in the rule itself. Instead, + individuals reading these texts will usually expect specific references to + *their codebase* and what can be done in order to improve it. + + +#### Contents and structure of documentation (KDoc) strings + +The description should be as detailed as possible as it will act as the +documentation of the rule. Incorporate the documentation string as KDoc +comment applied to the `Rule` subclass. + +Make use of `` and `` blocks to add +non-compliant and compliant code examples. Add these blocks right after +the detailed description of the rule. ```kotlin /** - * This is a nice description for the rule explaining what it checks, why it - * exists and how violations can be solved. + * Summary of the violation that this rule is concerned with, + * potentially extended by a brief description on why it is + * bad practice and what is usually done to eliminate it. + * + * Add more details if applicable... * * * // add the non-compliant code example here @@ -47,42 +142,145 @@ class SomeRule(config: Config = Config.empty) : Rule(config) { } ``` -The description should be as detailed as possible as it will act as the documentation of the rule. Add links to -references that explain the rationale for the rule if possible. +When authoring the documentation string for a rule, adhere to the following +guidelines: +1. The documentation string shall be formulated in such a manner that it refers + to the rule itself: + - :heavy_check_mark:: `Detects trailing spaces and recommends to remove them.` + - :x:: ``Trailing spaces detected. Consider removing them.`` +2. Stick to the [KDoc convention][3] that the first paragraph of documentation + text is the summary of the documented element (the rule), while the following + text is a detailed description. More specifically, populate these two regions + as follows: + - In the **summary**, give a concise description of the violation(s) that the + rule identifies. If applicable, a very brief summary of why the violation is + considered bad practice or what is usually done to eliminate it can be + given (but does not have to be given). + - In the **detailed description**, which does not have to be added for + simple/obvious rules, explain the violation(s) that the rule identifies in + greater detail. Add a rationale (ideally with a reference) outlining why such + a violation should be avoided. If applicable, add further remarks that make + it easier to understand or configure the rule. +3. Formulate all descriptions using (potentially incomplete) sentences with + proper capitalization and punctuation: + - :heavy_check_mark:: `Ensures that files with a single non-private class are named accordingly.` + - :x:: `ensures that files with a single non-private class are named accordingly` +4. Surround inline code (or code symbols) with `` ` `` characters (as in + [Markdown][4]): + - :heavy_check_mark:: ``Reports referential equality checks for types such as `String` and `List`.`` + - :x:: `Reports referential equality checks for types such as String and List.` + +#### General remarks on issue descriptions and code smell messages -The `` and `` code examples should be added right after the description of the rule. +Adhere to the following guidelines when authoring an issue description or code +smell messages: +1. Use a wording that focuses on the violation rather than on the rule itself. + Therefore, assume that the analyzed codebase contains at least one violation + of the respective rule. + - :heavy_check_mark:: ``Duplicated case statements in `when` expression.`` + - :x:: `Detects trailing spaces.` +2. Formulate the text as a sequence of (potentially incomplete) sentences with + proper capitalization and punctuation: + - :heavy_check_mark:: ``Duplicated case statements in `when` expression.`` + - :x:: ``duplicated case statements in `when` expression`` +3. Separate sentences from the sequence with one space. Most importantly, do not + use line breaks: + - :heavy_check_mark:: `Non-boolean property with prefix suggesting boolean type. This might mislead clients of the API.` + - :x:: `Non-boolean property with prefix suggesting boolean type.\nThis might mislead clients of the API.` +4. Surround inline code (or code symbols) with `` ` `` characters (as in + [Markdown][4]): + - :heavy_check_mark:: `` `map.get()` used with non-null assertion operator `!!`.`` + - :x:: `map.get() used with non-null assertion operator (!!).` -#### Rule annotations +*Note*: `non-null` is a borderline case. While `null` itself can be seen as +code, the prefix turns it into a conventional word. -```kotlin -@ActiveByDefault(since = "1.0.0") -@RequiresTypeResolution -class SomeRule(config: Config = Config.empty) : Rule(config) { +The above-mentioned guidelines tell you *how* to formulate issue descriptions +and code smell messages. To learn *what to include* in each of the texts, +refer to the following two sections. - @Configuration("This is the description for the configuration parameter below.") - private val name: String by config(default = "whatever should be the default") +#### Components of issue descriptions -} -``` +An issue description should consist of the following components: +1. **Violation** (*required*): What type of violation is it that detekt + identified in the codebase and, under consideration of the given detekt + configuration, recognizes as a violation of the considered rule? +2. **Rationale** (*optional*): Why is the identified violation considered an + issue? Especially for less experienced Kotlin developers, this might not be + clear from just the violation itself. +3. **Recommendation** (*optional*): What are developers usually expected to do + in order to eliminate the identified violation? In some cases, this might be + entirely obvious. If it is not, however, it makes sense to add a suitable + recommendation (or multiple alternative recommendations). -Use the `@Configuration` annotation in combination with the `config` delegate to create a configurable property for your rule. The name of the property will become the key and the provided default will be the value in the `default-detekt-config.yml`. All information are also used to generate the rule documentation in the wiki. -Note that a property that is marked with `@Configuration` must use the config delegate (and vice versa). +:warning: If possible, messages should first describe the violation, then the +optional rationale, and finally the optional recommendation. -Rules annotated with `@ActiveByDefault` will be marked as active in the `default-detekt-config.yml`. Generally this will not be the case for new rules. +*Exception*: If it makes sense in the specific case, it is also possible to deviate +from this order. Especially if the analyzed code contains a discouraged design +pattern or, alternatively, can be improved by adopting an encouraged design pattern, +the actual violation would often have to be described as 'making use of the +discouraged pattern' or as 'not making use of an encouraged design pattern'. In this +case, it is possible to focus on the rationale or the recommendation, while the +underlying violation is implicitly mentioned. -A rule that requires type resolution must be marked with `@RequiresTypeResolution`. See [the type resolution wiki page](../docs/pages/gettingstarted/type-resolution.md) for more detail on this topic. +:warning: In any case, try to keep the description as brief and concise as +possible! -The rule defined above will translate to a rule entry in the `default-detekt-config.yml`: -```yml -SomeRule: - active: true - name: 'whatever should be the default' -``` +The following list gives examples of compliant issue descriptions: +- :heavy_check_mark:: `Public library class.` → Just a violation. Compliant, but would certainly benefit from a few more details. +- :heavy_check_mark:: ``Public library class. Reduce its visibility to the module or the file.`` → Violation and recommendation. +- :heavy_check_mark:: `Function returns a constant. This is misleading.` → Violation and rationale. +- :heavy_check_mark:: `Function returns a constant, which is misleading. Use a constant property instead.` → All components. + +The following issue descriptions do not comply with this style guide: +- :x:: `Library class should not be public.` → Recommendation and violation (given implicitly). +- :x:: `A function that only returns a constant is misleading.` → Rationale and violation (given implicitly). + +Although the violation is implicitly described as part of the recommendation and +the rationale, respectively, these messages do not benefit enough from the +deviation (in comparison to the messages in the compliant examples above). + +In contrast, the following message does actually benefit from the implicit +description of the violation: +- :x:: ``The `next()` method of an `Iterator` implementation does not throw a `NoSuchElementException` when there are no more elements to return. In such situations, this Exception should be thrown.`` → Unnecessarily verbose. +- :heavy_check_mark:: ``The `next()` method of an `Iterator` implementation should throw a `NoSuchElementException` when there are no more elements to return.`` + → Recommendation and violation (given implicitly). + +#### Components of code smell messages +A code smell message should be dynamically created to describe a +*specific violation*. It should be regarded as an extension of the more generic +violation part of the issue description. More specifically, it should explicitly +reference identifiers or similar from the codebase. If it makes +sense in the specific context, it might be worthwhile to add a more detailed +version of the recommendation as well. If this is the case, add this extension +after the more specific description of the violation. -### When updating the website ... +:warning: Try to keep code smell messages even shorter than issue descriptions! -Make sure to test your changes locally. +The following list contains compliant and non-compliant examples of code smell +messages: +- :x:: ``Non-boolean property suggests a boolean type.`` + → Probably not more specific than the issue description. +- :heavy_check_mark:: ``Non-boolean property `hasXyz` suggests a boolean type.`` + → With a more specific description of the violation. +- :x:: ``Non-boolean property `hasXyz` suggests a boolean type. Remove the prefix.`` + → Pointless recommendation. +- :heavy_check_mark:: ``Non-boolean property `hasXyz` suggests a boolean type. Remove the `has` prefix.`` + → Specific recommendation. +- :heavy_check_mark:: ``Magic number `4` passed to `abc`. Pass it as a named argument.`` + → Usage-aware recommendation. + +*Note*: The recommendation given in the last example is usage-aware since +it is limited to cases in which a Kotlin function is called. Named +arguments [cannot be used when calling Java functions][5]. Since this +specific recommendation cannot be given for all encountered magic numbers, +it is not possible to incorporate it into the generic issue description. + +## Contributing to the website + +Make sure to test your changes locally: - install ruby and jekyll - gem install bundler @@ -90,19 +288,27 @@ Make sure to test your changes locally. - jekyll build - jekyll serve -Following warning is expected until [Jekyll](https://github.com/jekyll/jekyll/issues/7947) adopts to Ruby 2.7.0. +The following warning is expected until [Jekyll](https://github.com/jekyll/jekyll/issues/7947) adopts to Ruby 2.7.0: -`warning: Using the last argument as keyword parameters is deprecated (Ruby 2.7.0)` +``` +warning: Using the last argument as keyword parameters is deprecated (Ruby 2.7.0) +``` -### When working on the Gradle plugin ... +## Working on the Gradle plugin - Make changes to the core modules (e.g. adding a new CLI flag) - Run `gradle publishToMavenLocal` - Make changes to the Gradle plugin and add tests - Verify with `gradle detekt` -### Release process +## Releasing new detekt versions - `./scripts/github-milestone-report.main.kts` - creates changelog - `gradle increment` - update version - `./scripts/release.sh` - publish all artifacts + +[1]: https://github.com/detekt/detekt/blob/v1.19.0/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Issue.kt +[2]: https://github.com/detekt/detekt/blob/v1.19.0/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/CodeSmell.kt +[3]: https://kotlinlang.org/docs/kotlin-doc.html +[4]: https://daringfireball.net/projects/markdown/syntax +[5]: https://kotlinlang.org/docs/functions.html#named-arguments