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

Add require-number-to-fixed-digits-argument rule #1288

Merged
merged 14 commits into from
May 19, 2021

Conversation

fisker
Copy link
Collaborator

@fisker fisker commented May 18, 2021

Fixes #1245

@bmish
Copy link
Sponsor Contributor

bmish commented May 18, 2021

Some rule names in this plugin could use the common require- rule name prefix. I.e. require-number-to-fixed-digit-argument or require-array-join-separator.

@fisker
Copy link
Collaborator Author

fisker commented May 18, 2021

I thought about it and checked core rules and other plugins, but seems require- not very common, but I'm fine to add require-. Let's see what @sindresorhus think.

@bmish
Copy link
Sponsor Contributor

bmish commented May 18, 2021

I try to name almost all my lint rules with the following prefixes to make it clear what they are actually doing:

  • require-
  • no-
  • prefer-

@@ -0,0 +1,37 @@
# Enforce using the digits argument with `Number#toFixed()`

It's better to make it clear what the value of the `digits` argument is when calling [Number#toFixed()](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toFixed).
Copy link
Owner

Choose a reason for hiding this comment

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

We should mention the default here too, like #1294

@sindresorhus
Copy link
Owner

I agree. I don't think it's worth renaming existing rules, but we should try to follow that in the future.

@sindresorhus
Copy link
Owner

require-number-to-fixed-digit-argument

👍

@sindresorhus
Copy link
Owner

require-array-join-separator

We could do this one too, since it's not released yet.

@bmish
Copy link
Sponsor Contributor

bmish commented May 18, 2021

require-array-join-separator

We could do this one too, since it's not released yet.

Rename PR: #1295

@sindresorhus
Copy link
Owner

I try to name almost all my lint rules with the following prefixes to make it clear what they are actually doing:

Feel free to open a new issue with your suggestions on what rules should be renamed and we can consider it.

@fisker fisker changed the title Add number-to-fixed-digits rule Add require-number-to-fixed-digit-argument rule May 19, 2021
@fisker
Copy link
Collaborator Author

fisker commented May 19, 2021

I forgot another reason I didn't add require- at first place, if we want to do more in this rule, eg: check digits type, check it's 0-100 then the rule name won't fit. Same concern for require-array-join-seperator.

@bmish
Copy link
Sponsor Contributor

bmish commented May 19, 2021

I forgot another reason I didn't add require- at first place, if we want to do more in this rule, eg: check digits type, check it's 0-100 then the rule name won't fit. Same concern for require-array-join-seperator.

@fisker makes sense, I agree that if there's any chance we would want to generalize what the rule does, then your original name would be better. Feel free to revert if that's the case. However, in the past, I've also named rules require-valid-foo-argument in order to cover various possible checks that we could perform on that argument.

@fisker
Copy link
Collaborator Author

fisker commented May 19, 2021

Another example in core rules. The radix rule added check that number need to be integer 2-36, eslint/eslint#12608, the rule name didn't block that change. It even didn't mention parserInt

@fisker
Copy link
Collaborator Author

fisker commented May 19, 2021

require-valid-foo-argument

👍 , but for this rule, require-valid-number-to-fixed-digits-argument seems way too long

@fisker fisker changed the title Add require-number-to-fixed-digit-argument rule Add require-number-to-fixed-digits-argument rule May 19, 2021
@sindresorhus
Copy link
Owner

npm run generate-rules-table && git diff --exit-code is failing.

@fisker
Copy link
Collaborator Author

fisker commented May 19, 2021

I'll fix, what's your opinion about #1288 (comment)

@sindresorhus
Copy link
Owner

I forgot another reason I didn't add require- at first place, if we want to do more in this rule, eg: check digits type, check it's 0-100 then the rule name won't fit. Same concern for require-array-join-seperator.

I think in general we should try to think upfront before adding a rule whether we're likely to add more stuff to it in the future, if not, I think it's fine to use a specific name with require-*.

eg: check digits type

We can still do this even when it's named require-number-to-fixed-digits-argument.

@sindresorhus sindresorhus merged commit 4a30863 into sindresorhus:main May 19, 2021
@fisker fisker deleted the number-to-fixed-digits branch May 19, 2021 12:24
fisker added a commit that referenced this pull request May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: number-to-fixed-digits
3 participants