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

Make the version option a number instead of a string #59

Closed
sindresorhus opened this issue Mar 3, 2019 · 6 comments · Fixed by #79
Closed

Make the version option a number instead of a string #59

sindresorhus opened this issue Mar 3, 2019 · 6 comments · Fixed by #79

Comments

@sindresorhus
Copy link
Owner

@silverwind Why is it a string? It feels more natural for it to be a number.

@silverwind
Copy link
Collaborator

silverwind commented Mar 3, 2019

No real reason, but if we want to stay consistent with internal-ip, is-ip, is-cidr, ip-regex, cidr-regex, it would have to be methods, e.g. isOnline.{v4,v6}() and isOnline() being a separate method that possibly checks both (thought, I'm not sure if someone with only IPv6 working can be considered online, so probably best to keep it to v4 only).

@sindresorhus
Copy link
Owner Author

Having methods for it doesn't make sense here as it's an obscure option almost no one will need.

@sindresorhus
Copy link
Owner Author

This is not important, so we can just make sure to change to accept a number instead in the next major version.

@sindresorhus
Copy link
Owner Author

I would also like to rename the option to ipVersion as it's not obvious when just reading it that it's for IP.

@deadcoder0904
Copy link

@sindresorhus should I make version a number instead of a string & send a PR as it makes sense imo?

@sindresorhus
Copy link
Owner Author

@deadcoder0904 Nah, the issue is here so we can do the change if we need to do another major release. It's not doing a major release just for this.

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 a pull request may close this issue.

3 participants