Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update: Add automated suggestion to radix rule for parsing decimal numbers #14291

Merged
merged 1 commit into from Apr 30, 2021

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Apr 2, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?

radix

Does this change cause the rule to produce more or fewer warnings?

No.

How will the change be implemented? (New option, new default behavior, etc.)?

New automated suggestion.

What does the rule currently do for this code?

Does not provide an automated suggestion nor autofix.

What will the rule do after it's changed?

Provide an automated suggestion to change

parseInt('123');

to

parseInt('123', 10);

because parsing decimal numbers is the most common use case.

What changes did you make? (Give an overview)

Added a suggestion fixer function and tests.

Is there anything you'd like reviewers to focus on?

No.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Apr 2, 2021
@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

1 similar comment
@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@mdjermanovic mdjermanovic added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Apr 5, 2021
@mdjermanovic mdjermanovic added this to Evaluating in Triage Apr 5, 2021
@mdjermanovic
Copy link
Member

Thanks for the PR! I support this enhancement 👍

Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@fisker
Copy link
Contributor

fisker commented Apr 23, 2021

Is trailing comma case tested? parserInt(10,)

@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@bmish
Copy link
Sponsor Member Author

bmish commented Apr 23, 2021

Is trailing comma case tested? parserInt(10,)

@fisker good idea, added that test case.

@fisker
Copy link
Contributor

fisker commented Apr 24, 2021

Edge case parseInt( (0, "10") ) will fix to wrong code.

@nzakas
Copy link
Member

nzakas commented Apr 27, 2021

@fisker can you provide what it would be fixed to as well?

@fisker
Copy link
Contributor

fisker commented Apr 27, 2021

I think a better fix should check the penultimate token, if it's a comma token, insert 10,, if not insert , 10, both insert before the last token )

@bmish
Copy link
Sponsor Member Author

bmish commented Apr 27, 2021

I'll give that a try.

One other question, does it sound good for me to add suggestions for other common radix parameters?

  • binary (2)
  • octal (8)
  • hexadecimal (16)

@fisker
Copy link
Contributor

fisker commented Apr 27, 2021

Oh , I'm sorry, I fogot the second argument maybe already exists, maybe just check that fist argument isParenthesized is easier.

@fisker
Copy link
Contributor

fisker commented Apr 27, 2021

add suggestions for other common radix

I don't think that will be useful, if it's missing most time its's just forgot 10, but if it's not a valid radix I think most time it's just typo. My suggestion a little different #14356, anyway, I think just one suggestion with 10 is good enough.

@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The pull request title isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

Read more about contributing to ESLint here

@bmish
Copy link
Sponsor Member Author

bmish commented Apr 29, 2021

@fisker I added handling for the sequence expression. Good catch on that.

For now, I'll keep only one suggestion.

@fisker
Copy link
Contributor

fisker commented Apr 29, 2021

LGTM

@nzakas
Copy link
Member

nzakas commented Apr 30, 2021

@eslint/eslint-tsc can I get another pair of eyes on this?

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Good idea! LGTM

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Apr 30, 2021
@btmills btmills merged commit f071d1e into eslint:master Apr 30, 2021
Triage automation moved this from Evaluating to Complete Apr 30, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 28, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants