This repository was archived by the owner on Aug 31, 2023. It is now read-only.
docs(rome_js_analyze): improve CONTRIBUTING and code gen #4278
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
ematipico
reviewed
Mar 8, 2023
| will use: | ||
|
|
||
| ```shell | ||
| > just new-lintrule crates/rome_js_analyze/src/analyzers/nursery myRuleName |
Contributor
There was a problem hiding this comment.
One thing that could be improved is to have a link to how to install just. We take for granted that users can use it as it is.
Comment on lines
+28
to
+39
| When implementing **new rules**, they have to be implemented under the group `nursery`. | ||
| New rules should always be considered unstable/not exhaustive. | ||
|
|
||
| In addition to selecting a group, rules may be flagged as `recommended`. | ||
| The **recommended rules** are enabled in the default configuration of the _Rome_ linter. | ||
| As a general principle, a recommended rule should catch actual programming errors. | ||
| For instance detecting a coding pattern that will throw an exception at runtime. | ||
| Pedantic rules that check for certain unwanted patterns but may have high false positive rates, | ||
| should be left off from the recommended set. | ||
| Rules intended to be recommended should be flagged as such even if they are still part of the `nursery` group, | ||
| as unstable rules are only enabled by default on unstable builds. | ||
| This gives to the project time to test the rule, find edge cases, etc. |
Contributor
There was a problem hiding this comment.
Suggested change
| When implementing **new rules**, they have to be implemented under the group `nursery`. | |
| New rules should always be considered unstable/not exhaustive. | |
| In addition to selecting a group, rules may be flagged as `recommended`. | |
| The **recommended rules** are enabled in the default configuration of the _Rome_ linter. | |
| As a general principle, a recommended rule should catch actual programming errors. | |
| For instance detecting a coding pattern that will throw an exception at runtime. | |
| Pedantic rules that check for certain unwanted patterns but may have high false positive rates, | |
| should be left off from the recommended set. | |
| Rules intended to be recommended should be flagged as such even if they are still part of the `nursery` group, | |
| as unstable rules are only enabled by default on unstable builds. | |
| This gives to the project time to test the rule, find edge cases, etc. | |
| When implementing **new rules**, they have to be implemented under the group `nursery`. | |
| New rules should always be considered unstable/not exhaustive. | |
| In addition to selecting a group, rules may be flagged as `recommended`. | |
| The **recommended rules** are enabled in the default configuration of the _Rome_ linter. | |
| As a general principle, a recommended rule should catch actual programming errors. | |
| For instance, detecting a coding pattern that will throw an exception at runtime. | |
| Pedantic rules that check for specific unwanted patterns but may have high false positive rates | |
| should be left off from the recommended set. | |
| Rules intended to be recommended should be flagged as such even if they are still part of the `nursery` group, | |
| as unstable rules are only enabled by default on unstable builds. | |
| This gives the project time to test the rule, find edge cases, etc. |
Comment on lines
+43
to
+45
| When creating or updating a lint rule, you need to be aware that there's a lot of generated code inside our toolchain. | ||
| Our CI makes sure that this code is not out of sync and fails otherwise. | ||
| See the [code generation section](#code-generation) for more details. |
Contributor
There was a problem hiding this comment.
Suggested change
| When creating or updating a lint rule, you need to be aware that there's a lot of generated code inside our toolchain. | |
| Our CI makes sure that this code is not out of sync and fails otherwise. | |
| See the [code generation section](#code-generation) for more details. | |
| When creating or updating a lint rule, you need to be aware that there's a lot of generated code inside our toolchain. | |
| Our CI ensures this code is not out of sync and fails otherwise. | |
| See the [code generation section](#code-generation) for more details. |
Comment on lines
+110
to
+112
| While implementing the diagnostic, please keep [Rome's technical principals](https://rome.tools/#technical) in mind. | ||
| This function is called of every signal emitted by the `run` function, and it may return | ||
| zero or one diagnostic. |
Contributor
There was a problem hiding this comment.
Suggested change
| While implementing the diagnostic, please keep [Rome's technical principals](https://rome.tools/#technical) in mind. | |
| This function is called of every signal emitted by the `run` function, and it may return | |
| zero or one diagnostic. | |
| While implementing the diagnostic, please keep [Rome's technical principals](https://rome.tools/#technical) in mind. | |
| This function is called for every signal emitted by the `run` function, and it may return | |
| zero or one diagnostic. |
Comment on lines
+123
to
+126
| This function is called of every signal emitted by the `run` function. | ||
| It may return zero or one code action. | ||
| When returning a code action, you will need to pass `category` and `applicability` fields. | ||
| `category` must be `ActionCategory::QuickFix`, while `applicability` must be `Applicability:MaybeIncorrect`. |
Contributor
There was a problem hiding this comment.
Suggested change
| This function is called of every signal emitted by the `run` function. | |
| It may return zero or one code action. | |
| When returning a code action, you will need to pass `category` and `applicability` fields. | |
| `category` must be `ActionCategory::QuickFix`, while `applicability` must be `Applicability:MaybeIncorrect`. | |
| This function is called for every signal emitted by the `run` function. | |
| It may return zero or one code action. | |
| When returning a code action, you must pass the `category` and `applicability` fields. | |
| `category` must be `ActionCategory::QuickFix`, while `applicability` must be `Applicability:MaybeIncorrect`. |
must be seems to be a stretch; I mean, we have rules that have Applicability::Always. Maybe we should set more context of why we must set these values.
| `category` must be `ActionCategory::QuickFix`, while `applicability` must be `Applicability:MaybeIncorrect`. | ||
|
|
||
| ### Code generation | ||
| Don't forget to format your code with `cargo fmt` and lint with `cargo clippy`. |
Contributor
There was a problem hiding this comment.
Suggested change
| Don't forget to format your code with `cargo fmt` and lint with `cargo clippy`. | |
| Don't forget to format your code with `cargo format` and lint with `cargo lint`. |
Better use our internal aliases because we enforce more rules.
|
|
||
| When a rule's sole intention is to **mandate a single concept** - such as forcing the use of camel-casing - the rule should be named using the `use` prefix. For example, the rule to mandating the use of camel-cased variable names is named `useCamelCase`. | ||
| Note that code in a file ending with the extension `.jsonc` are in a _script environment_. | ||
| That's mean that you cannot use syntax that belongs to ECMAScript modules such as `import` and `export`. |
Contributor
There was a problem hiding this comment.
That's => This means that...
ematipico
pushed a commit
that referenced
this pull request
Mar 10, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves the internal documentation for the analyzer in several ways:
The PR also marginally improve the code generation for
just new-lintrule.Test Plan
No change.