fix(rome_js_analyze): fix noShoutyConstants with lowercase and being exported #3081
Conversation
Deploying with
|
| Latest commit: |
50c3ef7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://344729b1.tools-8rn.pages.dev |
| Branch Preview URL: | https://fix-no-shouty-constants-lowe.tools-8rn.pages.dev |
Parser conformance results on ubuntu-latestjs/262
jsx/babel
symbols/microsoft
ts/babel
ts/microsoft
|
|
!bench_analyzer |
Analyzer Benchmark Results |
|
A few suggestions:
|
| Some((id.clone(), literal.clone())) | ||
| } else { | ||
| None | ||
| for (from_id, from_literal) in id_text.chars().zip(literal_text.chars()) { |
There was a problem hiding this comment.
Technically both id_text and literal_text only ever cover a single token (IDENT and JS_STRING_LITERAL), so I think it would be possible to run this check over two &str directly rather than having to implement these helpers for SyntaxNodeText
There was a problem hiding this comment.
Done.
But I have mixed feelings about this:
- On one side is probably faster.
- But we are also not using the typed AST here. So I have to take care around quotes.
| if from_id != from_literal { | ||
| return None; | ||
| } | ||
| if from_id.is_lowercase() || from_literal.is_lowercase() { |
There was a problem hiding this comment.
I'm not sure the exit condition for the loop is correct here, from the description of the rule I would expect it to raise a diagnostic for const foo = "FOO"
There was a problem hiding this comment.
That is a good question. It is my understanding that the rule is only for uppercase bindings. const foo = "FOO" seems fine for me because it correctly gives a camel case name to a constant.
There was a problem hiding this comment.
Yeah the rule should work only for uppercase cases. That was its initial intention
We can also pull the valid cases from the original rule in Rome JS: https://github.com/rome/tools/blob/archived-js/website/src/docs/lint/rules/js/noShoutyConstants.md |
Done. |
…exported (rome#3081) * fix noShoutyConstants with lowercase and being exported
Summary
Closes #3077.
Test Plan
with two new cases