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

feat(rome_js_analyze): Implement prefer-numeric-literals lint #3558

Merged
merged 51 commits into from
Nov 14, 2022
Merged

feat(rome_js_analyze): Implement prefer-numeric-literals lint #3558

merged 51 commits into from
Nov 14, 2022

Conversation

95th
Copy link
Contributor

@95th 95th commented Nov 3, 2022

Implements:

Summary

Support prefer-numeric-literals lint rule

Test Plan

added unit tests for lint.

@95th 95th requested a review from a team November 3, 2022 20:44
@netlify
Copy link

netlify bot commented Nov 3, 2022 •
edited
Loading

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d302f51
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/637210d74d725200096bfbce
😎 Deploy Preview https://deploy-preview-3558--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@MichaReiser MichaReiser linked an issue Nov 4, 2022 that may be closed by this pull request
prefer-numeric-literals #3360
Closed
@95th
Copy link
Contributor Author

95th commented Nov 4, 2022 •
edited
Loading

For some reason, the below case is not caught (perhaps due to another rule already being applied)

Number[`parseInt`]('12', 8);

nvm. It was bug in the logic.

@95th
Copy link
Contributor Author

95th commented Nov 5, 2022

@MichaReiser I am getting the below error on the below test case. Can you please help?

Number.parseInt('17', 8)in foo

Error:

thread 'specs::style::prefer_numeric_literals_js' panicked at 'There should be no errors in the file "/Users/gurwindersi/tools/crates/rome_js_analyze/tests/specs/style/preferNumericLiterals.js" but the following errors where present:
/Users/gurwindersi/tools/crates/rome_js_analyze/tests/specs/style/preferNumericLiterals.js:63:1 parse ━━━━━━━━━━

  × expected a statement but instead found '0b11in foo
    Number.parseInt('17', 8)in foo
    parseInt('A', 16)in foo
    parseInt('11', 2) in foo
    Number.parseInt('17', 8)/**/in foo
    (parseInt('A', 16))in foo
    /* comment */Number.parseInt('11', 2)'

    61 │ Number.parseInt('17', 8)+5
    62 │ parseInt('A', 16)+5
  > 63 │ 0b11in foo
       │ ^^^^^^^^^^
  > 64 │ Number.parseInt('17', 8)in foo
        ...
  > 68 │ (parseInt('A', 16))in foo
  > 69 │ /* comment */Number.parseInt('11', 2);
       │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    70 │ Number/**/.parseInt('11', 2);

@95th 95th removed the request for review from xunilrj November 5, 2022 19:32
@95th 95th requested a review from MichaReiser November 8, 2022 18:38
MichaReiser
MichaReiser reviewed Nov 9, 2022
View reviewed changes
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated Show resolved Hide resolved
crates/rome_js_syntax/src/expr_ext.rs Outdated
/// Return the string value if the given expression is
/// 1. A string literal
/// 2. A template literal with no substitutions
pub fn as_string_or_no_substitution_template(&self) -> Option<String> {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to have doctest for all public facing methods.

crates/rome_js_analyze/src/ast_utils.rs Outdated Show resolved Hide resolved
crates/rome_js_analyze/src/react.rs Outdated Show resolved Hide resolved
@95th 95th requested a review from a team November 10, 2022 08:24
@MichaReiser
Copy link
Contributor

Thank you so much @95th for this contribution. Looking all good. Can you rebase your changes to resolve the merge conflicts, and we're good to

@MichaReiser MichaReiser merged commit c4797d3 into rome:main Nov 14, 2022
jeysal added a commit to jeysal/rometools that referenced this pull request Nov 14, 2022
@jeysal
Merge remote-tracking branch 'upstream/main' into no-dupe-keys
1b0f6fa 
* upstream/main: (45 commits)
  website(docs): set `color-scheme` on the root element (rome#3721)
  feat(rome_analyze): add a warning for unused suppression comments (rome#3718)
  feat(rome_js_analyze): Implement prefer-numeric-literals lint (rome#3558)
  feat(rome_js_formatter): jestEach template literals rome#3308 (rome#3582)
  doc(website): Add context about Romes philosophy (rome#3714)
  fix(rome_js_formatter): Single-line comment below a JSX prop triggers… (rome#3641)
  test(rome_js_formatter): update prettier tests (rome#3684)
  fix(rome_js_parser): improve await handling in non-async context (rome#3573)
  fix(rome_js_parser): improve yield parsing in non generator function (rome#3622)
  More playground polish
  Fix backgrounds
  Fix height
  Align docs.rome.tools with rome.tools
  Reenable compression
  Add missing width
  website(docs): More playground IDE features (rome#3711)
  fix(rome_js_formatter): new expression attribute (rome#3686)
  docs(website): added checkbox to toggle linter in playground (rome#3699)
  website(docs): More website tweaks (rome#3707)
  website(docs): Add default layout property (rome#3705)
  ...
@95th 95th deleted the prefer_numeric_literal branch November 18, 2022 04:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@leops leops leops left review comments

@MichaReiser MichaReiser MichaReiser approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

prefer-numeric-literals
3 participants
@95th @MichaReiser @leops