This repository has been archived by the owner on Aug 31, 2023. It is now read-only.
fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. #3789
Conversation
This file contains 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 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
mzbac
commented
Nov 18, 2022
crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
Outdated
Show resolved
Hide resolved
|
@MichaReiser Current fix doesn't change the behaviour. I am not very sure if we need to change the rule to ignore the shouty constant if it is used multiple times. |
MichaReiser
reviewed
Nov 18, 2022
There was a problem hiding this comment.
My understanding is that mainly flags #3658 the issue where a constant is used more than once. Inlining is then undesired because it would result in repeated code.
crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
Outdated
Show resolved
Hide resolved
|
Cool, I will update PR to include that change, Thanks |
mzbac
changed the title
ematipico
changed the title
added 7 commits
November 19, 2022 00:18
mzbac
commented
Nov 20, 2022
crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
Show resolved
Hide resolved
mzbac
commented
Nov 20, 2022
crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
Outdated
Show resolved
Hide resolved
MichaReiser
reviewed
Nov 21, 2022
crates/rome_js_analyze/src/semantic_analyzers/style/no_shouty_constants.rs
Outdated
Show resolved
Hide resolved
MichaReiser
reviewed
Nov 23, 2022
| @@ -145,15 +151,36 @@ impl Rule for NoShoutyConstants { | |||
| batch.remove_js_variable_declarator(ctx.query()); | |||
|
|
|||
| for reference in state.references.iter() { | |||
There was a problem hiding this comment.
It should be possible to simplify the state now that we always have exactly one reference:
Change: references to reference: Reference
There was a problem hiding this comment.
Thanks @MichaReiser Updated the PR as you suggested..
MichaReiser
reviewed
Nov 23, 2022
| ); | ||
|
|
||
| batch.replace_element(node.into_syntax().into(), new_element.into_syntax().into()); | ||
| } |
There was a problem hiding this comment.
One last change. We must return None if the reference is neither a IdentifierExpression nor a ShorthandProperty to avoid that we return an action that doesn't perform any changes.
There was a problem hiding this comment.
@MichaReiser 👍 , you are right. I just pushed the change. thanks
MichaReiser
approved these changes
Nov 24, 2022
jeysal
added a commit
to jeysal/rometools
that referenced
this pull request
Nov 24, 2022
* upstream/main: (73 commits) fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789) feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791) chore: run rustfmt and typo fix (rome#3840) feat(rome_js_analyze): use exhaustive deps support properties (rome#3581) website(docs): Fix text formatting (rome#3828) feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806) feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812) fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834) feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797) fix(rome_lsp): lsp friendly catch unwind (rome#3740) feat(rome_js_semantic): model improvements (rome#3825) feat(rome_json_parser): JSON Lexer (rome#3809) feat(rome_js_analyze): implement `noDistractingElements` (rome#3820) fix(rome_js_formatter): shothanded named import line break with default import (rome#3826) feat(rome_js_analyze): `noConstructorReturn` (rome#3805) feat(website): change enabledNurseryRules to All/Recommended select (rome#3810) feat(rome_js_analyze): noSetterReturn feat(rome_js_analyze): noConstructorReturn feat(rome_analyze): suppress rule via code actions (rome#3572) feat(rome_js_analyze): `noVar` (rome#3765) ...
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 is for fixing #3658. The current implementation fires false errors on exported shouty constants, and ignore the shouty constant checking when it's used more than once.
Test Plan
added new test cased to cover the fix