Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

add unsafeNegation to lint for in and instanceof #87

Merged
merged 5 commits into from
Mar 2, 2020

Conversation

macovedj
Copy link
Contributor

@macovedj macovedj commented Mar 1, 2020

I implemented the rule as eslint does without options. Happy to add other binary expressions if desired. Not sure if you want a PR per rule or not.

@facebook-github-bot
Copy link

Hi @macovedj!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@macovedj macovedj changed the title WIP: add unsafeNegotiation to lint for in and instanceof add unsafeNegotiation to lint for in and instanceof Mar 1, 2020
@macovedj macovedj mentioned this pull request Mar 1, 2020
@macovedj macovedj changed the title add unsafeNegotiation to lint for in and instanceof add unsafeNegation to lint for in and instanceof Mar 1, 2020
@macovedj macovedj requested a review from sebmck March 2, 2020 01:29
@sebmck
Copy link
Contributor

sebmck commented Mar 2, 2020

Sorry, one last thing. Could you add a small test here. Just another snapshot test that tests something basic. We can work on more test cases later. Thank you!

@macovedj
Copy link
Contributor Author

macovedj commented Mar 2, 2020

@sebmck

Sorry, one last thing. Could you add a small test here. Just another snapshot test that tests something basic. We can work on more test cases later. Thank you!

Just took a look and my understanding is that the example string can get formatted if we want. Looks like eslint takes the stance that it's rare that anybody would want to negate the left operand, so if we decided to adopt the same philosophy, then this seems like an opportune time to implement it in generateJS in the lint api. They allow for ignoring though, and it seems like we'd only want to to that if rome allows ignoring, and I haven't come across that yet. Thoughts? Maybe I'm overcomplicating things and I could just push up something simple for now like you said.

@macovedj
Copy link
Contributor Author

macovedj commented Mar 2, 2020

@sebmck You can probably disregard my earlier comment about tests. Just pushed a simple snapshot test.

@sebmck sebmck merged commit a17f7ee into rome:master Mar 2, 2020
@sebmck
Copy link
Contributor

sebmck commented Mar 2, 2020

Looks good, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants