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

Update: optionally allow bitwise operators (fixes #4742) #4786

Merged
merged 1 commit into from
Dec 31, 2015

Conversation

Swaagie
Copy link

@Swaagie Swaagie commented Dec 22, 2015

Mostly useful for allowing the NOT operator for code like ~[1, 2, 3].indexOf(1), which is something where using bitwise operators makes sense. I know there are no docs attached, but mainly was wondering if these kinds of features get accepted.

Usage would be along the line of no-bitwise: [2, { allow: [ '~' ] }]

edit: clean new PR from #4738
edit: related to #4742

* @param {ASTNode} node The node to check.
* @returns {boolean} Whether or not the node has a bitwise operator.
*/
function allowedOperator(node) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about isOperatorAllowed as the name of the function? Then pass in the operator as the argument to his function. Its more readable IMO.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to stick with the method signature already present in this rule, but if you feel strong about it I don't mind changing.

@gyandeeps
Copy link
Member

Please update:

  • Documentation of the rule with the new option.
  • Also update the rule schema to reflect the new option.

@gyandeeps
Copy link
Member

@btmills In the above case, option schema was not updated with the new option. I thought the unit test would fail in this case as the schema is not correct. Am i missing something here?

@btmills
Copy link
Member

btmills commented Dec 22, 2015

Good catch, @gyandeeps - that's a bug. I'll open an issue once I've diagnosed it further. It shouldn't block this PR - we just need to make sure the schema gets updated before we merge. As long as it's not [], the tests will catch any problems with it.

@Swaagie
Copy link
Author

Swaagie commented Dec 22, 2015

Good catch totally forgot about the docs, will add

@Swaagie
Copy link
Author

Swaagie commented Dec 22, 2015

Not sure about the schema, is there any task/command I can trigger to test if the schema's get updated or not?

@btmills
Copy link
Member

btmills commented Dec 22, 2015

It should be something like this. As long as the schema isn't [], it'll be validated as part of the tests.

module.exports.schema = [
    {
        "type": "object",
        "properties": {
            "allow": {
                "type": "array",
                "items": {
                    "enum": ["^", "|", "&", "<<", ">>", ">>>", "^=", "|=", "&=", "<<=", ">>=", ">>>=", "~"]
                },
                "uniqueItems": true
            }
        },
        "additionalProperties": false
    }
]

@Swaagie
Copy link
Author

Swaagie commented Dec 22, 2015

Added the schema, tests all run fine.

@michaelficarra
Copy link
Member

For anyone reading this PR and not paying attention to the original issue, please hold off on merging until my concerns there are addressed. I think this is entirely the wrong approach.

@indexzero
Copy link

+1 on this. This is one of the major "gotchas" that block many folks from switching from JSHint to ESLint. In JSHint the no-bitwise: true rule is equivalent to no-bitwise: [2, { allow: [ '~' ] }] in ESLint.

@michaelficarra after reading through #4742 it is clear that you disagree with this approach. That is noted. If you have an alternative approach you would like to suggest a PR speaks much more strongly than comments.

@nzakas @gyandeeps are we good here? I'd love to see this land. It's one of the last gotchas for more widespread ESLint adoption at GoDaddy because we switched from JSHint.

@nzakas
Copy link
Member

nzakas commented Dec 31, 2015

Yeah, I think this is good to go. There haven't been any other proposals and I still feel this is a good solution.

nzakas added a commit that referenced this pull request Dec 31, 2015
Update: optionally allow bitwise operators (fixes #4742)
@nzakas nzakas merged commit fd18d4e into eslint:master Dec 31, 2015
@indexzero
Copy link

Thanks @nzakas. Happy New Year! 💯

@Swaagie
Copy link
Author

Swaagie commented Jan 6, 2016

Thanks for merging this :)

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants