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

no-implicit-coercion: Separate options for !!x and ~x.indexOf("y") [$15] #3308

Closed
cvrebert opened this Issue Aug 7, 2015 · 18 comments

Comments

Projects
None yet
7 participants
@cvrebert
Copy link
Contributor

cvrebert commented Aug 7, 2015

Currently, the boolean option of no-implicit-coercion controls both of these. I think it would be nice to be able to control/(dis)allow these separately.
Personally, I find !! tolerable but the ~ hack too obscure.

Did you help close this issue? Go claim the $15 bounty on Bountysource.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Aug 7, 2015

Thanks for the issue! We get a lot of issues, so this message is automatically posted to each one to help you check that you've included all of the information we need to help you.

Reporting a bug? Please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. The source code that caused the problem
  3. The configuration you're using (for the rule or your entire config file)
  4. The actual ESLint output complete with line numbers

Requesting a new rule? Please be sure to include:

  1. The use case for the rule - what is it trying to prevent or flag?
  2. Whether the rule is trying to prevent an error or is purely stylistic
  3. Why you believe this rule is generic enough to be included

Requesting a feature? Please be sure to include:

  1. The problem you want to solve (don't mention the solution)
  2. Your take on the correct solution to problem

Including this information in your issue helps us to triage it and get you a response as quickly as possible.

Thanks!

@gyandeeps gyandeeps added the triage label Aug 7, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 11, 2015

Do you have a proposal for how this could work?

@cvrebert

This comment has been minimized.

Copy link
Contributor Author

cvrebert commented Aug 11, 2015

I suppose, for backward compatibility, allow boolean to accept either a single boolean or an object as its value. The new object would be something like (using strawman names) {"not-not": true, "bitwise-not": false}, thus allowing control over !!x and ~x.indexOf("y") (respectively).
Setting boolean to a boolean value would set both of these sub-settings to the same value, thus retaining backward compatibility.

@awerlang

This comment has been minimized.

Copy link

awerlang commented Aug 12, 2015

I agree to this proposed solution.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 12, 2015

Interesting idea, though I think perhaps using the actual character y !! and ~ might be clearer?

@cvrebert

This comment has been minimized.

Copy link
Contributor Author

cvrebert commented Aug 12, 2015

Sure, sounds fine. My names were just examples; I'n not attached to them at all.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 13, 2015

So what we're looking at is a configuration that looks like:

no-implicit-coercion: [2, {
    boolean: {
        "!!": false,
        "~": false
    }
}

Looking at it again, I think this is confusing. So maybe this instead:

no-implicit-coercion: [2, {
    exceptions: ["!!", "~"],
    boolean: true
}

Thoughts?

@cvrebert

This comment has been minimized.

Copy link
Contributor Author

cvrebert commented Aug 13, 2015

Looking at it again, I think this is confusing.

How so?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Aug 13, 2015

Because:

  1. You'd need to know that these are boolean coercions
  2. You'd then expect to be able to do the same in the other sections
  3. How would you set the default for boolean?
@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 16, 2015

I think we should go with exceptions to stay consistent with other rules.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Sep 17, 2015

Or allow to really go in the direction we're heading with other rules?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Sep 17, 2015

Yes, allow is probably better in this case. As in allow this pattern.

@nzakas nzakas added the help wanted label Sep 18, 2015

@nzakas nzakas changed the title no-implicit-coercion: Separate options for !!x and ~x.indexOf("y") no-implicit-coercion: Separate options for !!x and ~x.indexOf("y") [$15] Sep 26, 2015

@nzakas nzakas added the bounty label Sep 26, 2015

@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 13, 2016

Let me know if anyone else has already picked it. If not, I am going to work on it tonight.

Other than the specified two operators ~ and !!, would we like to allow other operators say +, * and also "" +?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

Yes, we should allow overrides for any of the operators we currently flag.

@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 13, 2016

How do we specify + differently, one for "" + stringVariable (empty string concat) and other for +booleanVariable (unary)?

@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 13, 2016

I am done with my changes except that empty string contact + operator overridability.

Example configuration that allows !!:

no-implicit-coercion: [2, {
    boolean: true,
    string: true,
    number: true,
    allow: ["!!", /*"~", "*", "+"*/]
}
@rpatil26

This comment has been minimized.

Copy link
Contributor

rpatil26 commented Jan 13, 2016

@nzakas Please let me know so that I can move on and create PR. I couldn't come up with any reliable way to specify unary and binary +es separately.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jan 13, 2016

We probably need to allow all uses of + and just not worry about differentiating. Just make a note of it in the docs.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 14, 2016

Update: Add "allow" option to allow specific operators (fixes eslint#…
…3308)

New "allow" option has been introduced to allow specific operators.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 14, 2016

Update: Add "allow" option to allow specific operators (fixes eslint#…
…3308)

New "allow" option has been introduced to allow specific operators.

rpatil26 added a commit to rpatil26/eslint that referenced this issue Jan 14, 2016

Update: Add "allow" option to allow specific operators (fixes eslint#…
…3308)

New "allow" option has been introduced to allow specific operators.

@nzakas nzakas closed this in 700b8bc Jan 14, 2016

nzakas added a commit that referenced this issue Jan 14, 2016

Merge pull request #4944 from rpatil26/issue3308
Update: Add "allow" option to allow specific operators (fixes #3308)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.