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 prefer-to-be-defined rule #8

Closed

Conversation

xfumihiro
Copy link
Contributor

No description provided.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

I'll ask the same question here I did in the Jest repo, should this tule be combined with the undefined rule? Does it make sense separately?


## Rule details

This rule triggers a warning if `toBe()` is used to assert a undefined value.
Copy link
Member

Choose a reason for hiding this comment

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

not.toBe()

The following patterns are considered warning:

```js
expect(true).not.toBe(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should warn, that should be caught by the other rule.

As eslint fixes are multipass, you'd get the same result in the end when autofixing, and then you don't have 2 rules warning about the same thing at once 🙂

const parentProperty2 = node.parent.parent.property;
const propertyName2 = parentProperty2.name;
const argument = node.parent.parent.parent.arguments[0];
const propertyDot = context
Copy link
Member

Choose a reason for hiding this comment

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

put this inside of the fixer so we don't do it every time

);
if (
(propertyName === 'not' &&
propertyName2 === 'toBe' &&
Copy link
Member

Choose a reason for hiding this comment

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

and toEqual

'use strict';

const RuleTester = require('eslint').RuleTester;
const { rules } = require('../../');
Copy link
Member

@SimenB SimenB Nov 10, 2017

Choose a reason for hiding this comment

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

Can't use destructuring (failing CI)

@xfumihiro xfumihiro force-pushed the prefer-to-be-defined branch 4 times, most recently from 6ccdf5c to 73894e2 Compare November 11, 2017 01:28

invalid: [
{
code: 'expect(true).not.toBe(undefined);',
Copy link
Member

Choose a reason for hiding this comment

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

See #8 (comment), I don't think this rule should warn on toBeUndefined(), the other rule can warn on that

@SimenB
Copy link
Member

SimenB commented Nov 24, 2017

@xfumihiro This needs a rebase.

Can you also update the changelog? 🙂


invalid: [
{
code: 'expect(true).not.toEqual(undefined);',
Copy link
Member

Choose a reason for hiding this comment

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

Should have a test for toBe as well

const ruleTester = new RuleTester();

ruleTester.run('prefer_to_be_defined', rules['prefer-to-be-defined'], {
valid: ['expect(true).toBeDefined();'],
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some more examples here? Mostly to be sure we avoid stuff like #6 and #20

@SimenB
Copy link
Member

SimenB commented Nov 26, 2017

I like the direction! Definitely something we can build upon.

It's currently failing on Node 4, as that does not support destructuring.

@xfumihiro
Copy link
Contributor Author

Isn't Node 4 support being dropped?

@thymikee
Copy link
Member

Yep, in the next release.

@SimenB
Copy link
Member

SimenB commented Nov 26, 2017

It's being dropped for jest core in the next release, not necessarily here 🙂 See #1 where it was discussed

@SimenB
Copy link
Member

SimenB commented Nov 29, 2017

We could add babel, but it is nice to not have to do that

@xfumihiro
Copy link
Contributor Author

I agree it makes more sense that this should be merged with prefer-to-be-undefined rule, but what name should it have?
prefer-to-be-defined-or-undefined?
prefer-to-be-defined-undefined?
prefer-to-be-un-defined?

@SimenB
Copy link
Member

SimenB commented Dec 3, 2017

What about just to-be-defined? I'm not sure if the un part needs to be explicit.

@SimenB
Copy link
Member

SimenB commented Dec 4, 2017

Needs a rebase 🙂

@xfumihiro xfumihiro force-pushed the prefer-to-be-defined branch 2 times, most recently from ebc0615 to b6a59b9 Compare December 4, 2017 16:09
@SimenB
Copy link
Member

SimenB commented Dec 5, 2017

needs a rebase again, sorry.

@SimenB
Copy link
Member

SimenB commented Dec 5, 2017

Can you mark the commit with BREAKING CHANGE in the body when you rebase?

BREAKING CHANGE: prefer-to-be-undefined rule has been removed and merged
into this rule.
@G-Rath
Copy link
Collaborator

G-Rath commented Nov 9, 2019

@xfumihiro Are you still interested in championing this PR, as it would be cool to get this across the line :)

Since it was done before the typescript migration, it's probably best to create a new PR rather than rebase.

Otherwise if you're not interested (or don't have the time), I'm happy to pick up this PR.

@xfumihiro
Copy link
Contributor Author

@G-Rath go ahead plz :)

Base automatically changed from master to main March 6, 2021 19:29
@G-Rath
Copy link
Collaborator

G-Rath commented Apr 11, 2021

I'm going to close this off for now - overall I'm not against the rule, but this PR has been around for a while, and you should at least be able to archive a similar effect with no-restricted-matchers (specifically, you should be able to restrict using not.toBeUndefined in favor of toBeDefined).

@G-Rath G-Rath closed this Apr 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants