Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

fix(rome_js_analyze): accept const as non camel case #3190

Merged
merged 5 commits into from
Sep 11, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Sep 9, 2022

Summary

This PR fixes a false flag of all uppercase const as an invalid non-camel case.
Fixes a lot of issues here: https://github.com/rome/tools/runs/8188485278

Test Plan

> cargo test -p rome_js_analyze -- camel_case

@xunilrj xunilrj requested a review from leops as a code owner September 9, 2022 09:27
@netlify
Copy link

netlify bot commented Sep 9, 2022

Deploy Preview for rometools canceled.

Name Link
🔨 Latest commit c939931
🔍 Latest deploy log https://app.netlify.com/sites/rometools/deploys/631bafefae531e00080a4cc0

@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 09:27 Inactive
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you enable the rule inside rome.json file? Doing so it will be a better proof that this PR fixes the false positives

@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 13:01 Inactive
@xunilrj xunilrj requested a review from a team September 9, 2022 13:06
@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 13:06 Inactive
@@ -34,6 +34,8 @@ console.log({
let camelCase;
let longCamelCase;

const THIS_IS_OK = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix cover cases like const { FOO } = env?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I update the test with more cases.
There is one case we do not deal well.

warning[js/useCamelCase]: Prefer variables names in camel case.
   ┌─ useCamelCase.js:31:5
   │
31 │ let UPPER_CASE = 1;
   │     ----------

Safe fix: Rename this symbol to camel case
      | @@ -28,7 +28,7 @@
27 27 |   let camelCase;
28 28 |   let longCamelCase;
29 29 |   
30    | - let UPPER_CASE = 1;
   30 | + let uPPERCASE = 1;
31 31 |   let { UPPER_CASE } = 1;
32 32 |   let [ UPPER_CASE ] = 1;
33 33 |  

The problem is that it is easy to fix this, but break other acronyms like IS_HTML -> isHtml.
Not sure the best way here.

@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 15:53 Inactive
@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 15:56 Inactive
@xunilrj xunilrj force-pushed the fix/use-camel-case-consts branch from c93ccde to aba5332 Compare September 9, 2022 20:52
@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 20:52 Inactive
@xunilrj xunilrj force-pushed the fix/use-camel-case-consts branch from aba5332 to c939931 Compare September 9, 2022 21:28
@xunilrj xunilrj temporarily deployed to netlify-playground September 9, 2022 21:28 Inactive
@xunilrj xunilrj merged commit 0af2a22 into main Sep 11, 2022
@xunilrj xunilrj deleted the fix/use-camel-case-consts branch September 11, 2022 19:29
@xunilrj xunilrj added A-Linter Area: linter L-JavaScript Langauge: JavaScript labels Sep 14, 2022
@xunilrj xunilrj added this to the 0.10.0 milestone Sep 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A-Linter Area: linter L-JavaScript Langauge: JavaScript
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants