Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

feat!: activate the "radix" rule #66

Merged
merged 1 commit into from
Aug 14, 2019
Merged

feat!: activate the "radix" rule #66

merged 1 commit into from
Aug 14, 2019

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Aug 13, 2019

If we can't safely use parseInt(foo) everywhere, we should probably use it nowhere. Let's just use parseInt(foo, 10) consistently.

See: https://eslint.org/docs/rules/radix

If we can't safely use `parseInt(foo)` everywhere, we should probably
use it nowhere. Let's just use `parseInt(foo, 10)` consistently.

See: https://eslint.org/docs/rules/radix
@jbalsas
Copy link
Contributor

jbalsas commented Aug 14, 2019

Hey @wincent, where can't we use parseInt safely? I feel we might want to start collecting the rationale for some of our rules here. Maybe having some brief explanations as to why we enforce the rules we do would help people agree to them. We could link to here (if we haven't yet) from https://github.com/liferay/liferay-frontend-guidelines

@wincent
Copy link
Contributor Author

wincent commented Aug 14, 2019

where can't we use parseInt safely?

It's less important now that we're using Babel on "everything", but passing a base is the safe default as explained on MDN:

If the input string begins with "0" (a zero), radix is assumed to be 8 (octal) or 10 (decimal). Exactly which radix is chosen is implementation-dependent. ECMAScript 5 clarifies that 10 (decimal) should be used, but not all browsers support this yet. For this reason always specify a radix when using parseInt.

I'm thinking that because we don't necessarily know when or where code will be backported (for example to a branch where Babel isn't running on everything) that it's safest to just always require it. On the other hand, if you feel sure that we'll never ever run in such an environment, we can actually flip the rule to enforce the opposite. Either way though, it would be nice to have a rule that does this for us so that we don't have to think about it on a case-by-case basis.

@jbalsas
Copy link
Contributor

jbalsas commented Aug 14, 2019

Makes sense, let's write this down somewhere. I agree that enforcing radix seems safer for us at the moment.

@jbalsas jbalsas merged commit 032a64f into master Aug 14, 2019
@wincent wincent deleted the wincent/radix branch August 14, 2019 07:02
@wincent
Copy link
Contributor Author

wincent commented Aug 14, 2019

let's write this down somewhere.

For me the most natural thing to do if I want to know why a particular rule is in force would be to git blame on it to see when/why it was added. But I think we could add to the list of rules in the readme to make it a bit easier to find these rationales (eg. a link to whatever the "most appropriate" resource is: could be a PR, and issue, or some discussion in the frontend-guidelines repo etc). I'll send a PR showing what I mean.

wincent added a commit that referenced this pull request Aug 14, 2019
wincent added a commit that referenced this pull request Aug 14, 2019
wincent added a commit to liferay/liferay-npm-tools that referenced this pull request Aug 14, 2019
Grab the latest version of eslint-config-liferay, which brings:

    liferay/eslint-config-liferay#66

And also various liferay-js-toolkit packages, motivated because we want
to get this change into liferay-portal:

    liferay/liferay-js-toolkit#380
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants