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 opt-out options to the no-bitwise rule #4742

Closed
Swaagie opened this Issue Dec 18, 2015 · 16 comments

Comments

Projects
None yet
5 participants
@Swaagie
Copy link
Contributor

Swaagie commented Dec 18, 2015

Related to PR: #4738

Let me know how you think about the feature. The PR has a initial implementation to allow opting back in to individual operators.

@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Dec 18, 2015

@Swaagie Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 18, 2015

Hmm... I'm not sure about an option like that. On one hand I can see how it might be useful, on the other hand, bitwise operators are not something that most of the people are familiar with, and this rule was created specifically for those people. Can you explain why you wouldn't just turn off this rule on your codebase?

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 21, 2015

It's to catch unintentional uses of the bitwise operators. e.g. whenever devs might accidentally use if (arg >> -1) where arg=0 but wanted if (arg > -1). I don't think having the optional additions to allow some operators would affect those people that either want or don't want to use bitwise operators at all.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 21, 2015

I think the suggestion is fine, but I think the implementation in the PR is a bit confusing. If like to see something like this instead:

no-bitwise: [2, { allow: [ "!", ">>" ] }]

Using the actual operator make it very clear what is happening. @Swaagie do you want to update your PR to match?

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 22, 2015

That seems like a more sane way to do it, I'll change the logic

Swaagie added a commit to Swaagie/eslint that referenced this issue Dec 22, 2015

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 22, 2015

PR ready to be merged, properly respecting contribution guidelines

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 22, 2015

I don't think the allowance should be based on operator. You should look to allow the particular patterns you're interested in using, such as the one mentioned or the ones necessary for asm.js.

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 22, 2015

I'm not sure I'm following what your saying can you clarify?

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 22, 2015

You don't want an exception for ~a or ~~-~a, you just want an exception for ~a.indexOf(b), so make that the exception. You're currently asking to make the exception overly broad for no reason.

Swaagie added a commit to Swaagie/eslint that referenced this issue Dec 22, 2015

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 22, 2015

Isn't that way beyond the scope of the rule though, I can think of tons of patterns to use & or |, making all those an exceptions is a bit obtuse. Also I don't see why this optional configurability would hurt anyone, there is no need to use it

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 22, 2015

You have a particular case that is motivating your request for an exception to this rule. Instead of creating an exception that covers this case as narrowly as possible, you've proposed an exception that will accidentally also ignore cases that I'm sure even you yourself would want reported. You should greatly narrow the scope of this exception.

Swaagie added a commit to Swaagie/eslint that referenced this issue Dec 22, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 22, 2015

I'm the one who made the suggestion to allow any operator because we will undoubtedly get more requests like this. I'm not comfortable making a change that is so specific as to just allow ~a.indexOf(). People will now have the option of disabling the rule, or disabling the rule for one or more operators. That should be enough flexibility for most use cases.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Dec 22, 2015

@nzakas Just because I want to do a | 0 doesn't mean I want to be able to do a | b.It is entirely reasonable that everybody that wants an exception will not want the exception for every usage of that operator. If this will literally always be the case, why not accommodate it?

@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Dec 22, 2015

How would you suggest to accommodate to literally any random implementation logic out there though? I can see a lot of edge cases around differences in the ast. That said I'm fine if this is adjusted towards a more specific ruleset to define exceptions. However, it would be outside my scope/knowledge of the eslint code base.

Swaagie added a commit to Swaagie/eslint that referenced this issue Dec 22, 2015

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Dec 23, 2015

@michaelficarra if you'd like to propose an alternative, please do.

@nzakas nzakas closed this in 4931f56 Dec 31, 2015

nzakas added a commit that referenced this issue Dec 31, 2015

Merge pull request #4786 from Swaagie/allow-bitwise
Update: optionally allow bitwise operators (fixes #4742)
@Swaagie

This comment has been minimized.

Copy link
Contributor Author

Swaagie commented Jan 6, 2016

Thanks for merging this :) was out on vacation a bit

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

@eslint eslint bot added the archived due to age label Feb 6, 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.