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

fix is-number example #129

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

talentlessguy
Copy link

No description provided.

@43081j
Copy link
Contributor

43081j commented Aug 1, 2024

i'd like more opinions on this

it aligns the code with what is-number does, but you could quite easily argue what is-number does is a waste of time

the extra check is to make sure someone doesn't pass Infinity. I think that is the job of the consumer to decide if they care or not about Infinity, rather than us pushing it as the common use case

I suspect the majority would be fine with just typeof n === 'number' and, if they need to allow strings, typeof n === 'number' || !isNaN(n) (or if you want to avoid isNaN, Number.isNaN(Number(n)))

@talentlessguy
Copy link
Author

is-number does not actually check if a number is a number even... it check if a value is either a number or a number inside a string

@43081j
Copy link
Contributor

43081j commented Aug 1, 2024

indeed, which is what my second example would achieve: typeof n === 'number' || !isNaN(n)

the extra logic is checking if the number is Infinity and returning false if it is

i'm saying we probably don't need to do any of that because it isn't the common use case

@talentlessguy
Copy link
Author

I think it's best not to ask to replace with typeof, in case somebody's relying on string parsing

@43081j
Copy link
Contributor

43081j commented Aug 3, 2024

that is what || !isNaN(n) would achieve

maybe im not being clear enough, let me try explain again:

the code you've added here (and what is-number does) is doing two things:

  • n - n === 0 is basically checking it isn't Infinity and isn't NaN
  • v.trim() !== "" is accounting for the fact +"" is 0

what i'm saying is we shouldn't be emulating what is-number does. we should be pushing people to use simpler code

typeof n === 'number' || !isNaN(n) will allow literal numbers and number-like strings, without the extra cruft around checking for infinity and what not

recommend what you want to see used, rather than what is currently used

@talentlessguy
Copy link
Author

ah alright, sorry

now I get it

I'll update my PR accordingly

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.

2 participants