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

feat(eslint-plugin): add strict-comparisons rule #314

Conversation

AndreasGassmann
Copy link

Adds a strict-comparisons rule.

I also made a PR on the TSLint repo palantir/tslint#4519 which addresses an open issue palantir/tslint#3574. Once that one gets merged, I'll update the references in this PR.

I'm not sure about the name, I started calling it no-object-comparisons, but now it feels like it's doing more than just that, so strict-comparisons seems more fitting.

I'll have to migrate over some more tests from the TSLint repo, but because of the different format this takes quite some time.

@codecov
Copy link

codecov bot commented Feb 24, 2019

Codecov Report

Merging #314 into master will decrease coverage by 0.09%.
The diff coverage is 92.98%.

@@            Coverage Diff            @@
##           master     #314     +/-   ##
=========================================
- Coverage    97.2%   97.11%   -0.1%     
=========================================
  Files          68       69      +1     
  Lines        2505     2562     +57     
  Branches      387      393      +6     
=========================================
+ Hits         2435     2488     +53     
- Misses         44       46      +2     
- Partials       26       28      +2
Impacted Files Coverage Δ
...ages/eslint-plugin/src/rules/strict-comparisons.ts 92.98% <92.98%> (ø)

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

I can't give much of an in-depth review of the type code right now.
but test coverage and documentation looks good.

packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Mar 15, 2019
@AndreasGassmann
Copy link
Author

So I think from my side this would be ready. Is there anything else I should do? @bradzacher @mohsen1

The corresponding tslint PR is still pending due to some compatibility issues with TS 2.1 and some new linting errors in their codebase.

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

a few nits, otherwise LGTM.

It would be good to see some examples/docs clarifying what cases are valid with some more types.

i.e. it's not obvious from the docs (or tests) whether I can do this without the allowObjectEqualComparison option, because your examples mostly use literals:

const x : T | null = ...;
if (x === null) { .... }

packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/src/rules/strict-comparisons.ts Outdated Show resolved Hide resolved
bradzacher
bradzacher previously approved these changes Mar 24, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

great work!

@AndreasGassmann
Copy link
Author

@bradzacher thanks. I'll update the docs and examples to clarify the different cases, once that is done I think it's ready from my side.

@bradzacher bradzacher dismissed their stale review April 6, 2019 00:24

awaiting docs update

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

update the docs and fix the merge conflict and this LGTM

@bradzacher bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Apr 19, 2019
@JamesHenry
Copy link
Member

Hi @AndreasGassmann, thanks again for the PR, when do you think you will have chance to finalise this one?

@AndreasGassmann
Copy link
Author

Sorry for not working on this for so long. I'll look at it this week.

@bradzacher bradzacher added the help wanted Extra attention is needed label Nov 22, 2019
@bradzacher bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Apr 13, 2020
@bradzacher bradzacher closed this Apr 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
awaiting response Issues waiting for a reply from the OP or another party enhancement: new plugin rule New rule request for eslint-plugin help wanted Extra attention is needed stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants