Skip to content

ts-migration/prefer-called-with #346

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

ts-migration/prefer-called-with #346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ...ules/__tests__/prefer-called-with.test.js → ...ules/__tests__/prefer-called-with.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { RuleTester } from 'eslint';
import { TSESLint } from '@typescript-eslint/experimental-utils';
import rule from '../prefer-called-with';

const ruleTester = new RuleTester();
const ruleTester = new TSESLint.RuleTester();

ruleTester.run('prefer-called-with', rule, {
valid: [
Expand Down
26 changes: 15 additions & 11 deletions src/rules/prefer-called-with.js → src/rules/prefer-called-with.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,26 +1,30 @@
import {
expectCaseWithParent,
expectNotCase,
getDocsUrl,
method,
} from './util';
createRule,
isExpectCallWithNot,
isExpectCallWithParent,
} from './tsUtils';

export default {
export default createRule({
name: __filename,
meta: {
docs: {
url: getDocsUrl(__filename),
category: 'Best Practices',
description: 'Suggest using `toBeCalledWith` OR `toHaveBeenCalledWith`',
recommended: false,
},
messages: {
preferCalledWith: 'Prefer {{name}}With(/* expected args */)',
preferCalledWith: 'Prefer {{ name }}With(/* expected args */)',
},
type: 'suggestion',
schema: [],
},
defaultOptions: [],
create(context) {
return {
CallExpression(node) {
// Could check resolves/rejects here but not a likely idiom.
if (expectCaseWithParent(node) && !expectNotCase(node)) {
const methodNode = method(node);
if (isExpectCallWithParent(node) && !isExpectCallWithNot(node)) {
const methodNode = node.parent.property;
const { name } = methodNode;
if (name === 'toBeCalled' || name === 'toHaveBeenCalled') {
context.report({
Expand All @@ -33,4 +37,4 @@ export default {
},
};
},
};
});
39 changes: 37 additions & 2 deletions src/rules/tsUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,14 @@ export const createRule = ESLintUtils.RuleCreator(name => {
return `${REPO_URL}/blob/v${version}/docs/rules/${ruleName}.md`;
});

interface JestExpectIdentifier extends TSESTree.Identifier {
name: 'expect';
interface JestSpecificIdentifier<Name extends string>
extends TSESTree.Identifier {
name: Name;
}

type JestExpectIdentifier = JestSpecificIdentifier<'expect'>;
type JestNotIdentifier = JestSpecificIdentifier<'not'>;

/**
* Checks if the given `node` is considered a {@link JestExpectIdentifier}.
*
Expand Down Expand Up @@ -67,6 +71,16 @@ interface JestExpectCallWithParent extends JestExpectCallExpression {
parent: JestExpectCallMemberExpression;
}

interface JestNotNamespaceMemberExpression
extends JestExpectCallMemberExpression {
object: JestExpectCallExpression;
property: JestNotIdentifier;
}

interface JestExpectCallWithNotParent extends JestExpectCallWithParent {
parent: JestNotNamespaceMemberExpression;
}

export const isExpectCallWithParent = (
node: TSESTree.Node,
): node is JestExpectCallWithParent =>
Expand All @@ -75,6 +89,27 @@ export const isExpectCallWithParent = (
node.parent.type === AST_NODE_TYPES.MemberExpression &&
node.parent.property.type === AST_NODE_TYPES.Identifier;

/**
* Checks if the given `node` is a {@link JestExpectCallWithNotParent}.
*
* This is any call to `expect` that is followed by the `not` namespace:
*
* @example```
* expect('a').not.toBe('b');
* expect().not
* ```
*
* @param {Node} node
*
* @return {node is JestExpectCallWithNotParent}
*/
export const isExpectCallWithNot = (
Copy link
Member

@SimenB SimenB Jul 23, 2019

Choose a reason for hiding this comment

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

could possibly have a version with this that receives a JestExpectCallWithParent which just checks the parent? in this rule, we call isExpectCallWithParent twice. Probably not a big issue as the checks are pretty cheap

node: TSESTree.Node,
): node is JestExpectCallWithNotParent =>
Copy link
Contributor

@yatki yatki Jul 23, 2019

Choose a reason for hiding this comment

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

JestExpectCallWithNotParent looks like a Call without parent. Don't you think ExpectNotCall is a better naming @G-Rath :) Also we could avoid conflicts :D

Copy link
Collaborator Author

@G-Rath G-Rath Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe? iirc I don't think b/c the parent is the Not.

I've actually spent the whole day pretty much writing a completely typesafe implementation of the guards that are also completely compatible w/ identifiers, literals and template literals, and am now converting the remaining rules right now.

I hope to push them up pretty soon, but they're still somewhat messy.

There was always going to be naming conflicts unfortunately.

Just to give you a little taste of the new guards in action:

const fn2 = (node: TSESTree.Node) => {
  if (
    isExpectCallWithSpecificParent(node, 'not') &&
    isExpectCallWithSpecificGrandParent(node, 'toBe')
  ) {
    if (getAccessorExpressionValue(node.parent.parent.property) === 'not') {
        // ^^ TS error - this will never be true
    }
    
    if (getAccessorExpressionValue(node.parent.property) === 'resolves') {
    }

    if (getAccessorExpressionValue(node.parent.parent.property) === 'toBe') {
    }
  }
};

Copy link
Contributor

@yatki yatki Jul 23, 2019

Choose a reason for hiding this comment

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

oh looks nice.. Ok, no worries. Keep doing what you think is the best. As long as @SimenB is ok with these changes, later I can refactor my PR accordingly. 🖖

Copy link
Collaborator Author

@G-Rath G-Rath Jul 23, 2019

Choose a reason for hiding this comment

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

No problem :)

Your PR was a huge help, as it helped confirm a lot of things I wanted to confirm before trying to rewrite all the guards.

Would it be ok if I pulled your PR into mine? Just in case I need to to hit 100% coverage & ensure the guards are all correct.

I'm definitely going to want as many eyes as I can on it, as there's definitely redundancy, so I'll make sure to ping you when I push up a draft PR (which I might be doing very soon).

Copy link
Member

@SimenB SimenB Jul 23, 2019

Choose a reason for hiding this comment

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

I don't really have an opinion either way. In general I'd like to avoid the parent terminology since I find it weird. But I think we should use it for now since it represents the AST nodes, and keeping the names correct makes it easier to reason about. Once we've ported all rules and we've got strong type safety in the utils, I'd like to revisit #73 (comment). So you can some kind of custom structure back and instead of accessing .parent (or .parent.parent in case of not) you just do .matcher and check a boolean .isNot or something.

Anyways, rambling way of saying "these names are fine for now, I'd like to revisit after migration is complete to see if we can simplify" 🙂

Copy link
Collaborator Author

@G-Rath G-Rath Jul 23, 2019

Choose a reason for hiding this comment

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

I completely agree, and just you wait until you see some of the new names :D

Tbh, while the names seem to go against what we've taught as programmers, in AST land they're actually really nice - things like isExpectCallWithJestNamespace (here's a structural screenshot which contains about half the types & guards: http://puu.sh/DWjAi/e0a141eab4.png) that are actually really nicely verbose once you get use to using them in the land of AST where there's lots of annoying layers.

I do totally agree w/ the parent thing - it's super annoying, b/c I've not been able to think up of a better name that isn't less accurate or technically incorrect in some way :/

Copy link
Member

@SimenB SimenB Jul 23, 2019

Choose a reason for hiding this comment

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

For sure, I think we should keep the verbose names as they more directly map to the underlying AST nodes we operate on. But down the line we can create more human readable abstractions on top of it to easily make assertions about different matchers (while still keeping the verbose versions as the implementation)

Copy link
Collaborator Author

@G-Rath G-Rath Jul 23, 2019

Choose a reason for hiding this comment

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

I think that's exactly the right way to go about it, and what I've been working towards.

It's already how we're doing accessors now:

type SpecificAccessorExpression<Specifics extends string> =
  | SpecificStringNode<Specifics>
  | SpecificIdentifier<Specifics>;

type AccessorNode = SpecificAccessorExpression<string>;

const isSupportedAccessor = (node: TSESTree.Node): node is AccessorNode =>
  node.type === AST_NODE_TYPES.Identifier || isStringNode(node);

export const getAccessorExpressionValue = <S extends string = string>(
  accessor: SpecificAccessorExpression<S>,
): S =>
  accessor.type === AST_NODE_TYPES.Identifier
    ? accessor.name
    : getSpecificStringValue(accessor);

isExpectCallWithParent(node) &&
node.parent.object.type === AST_NODE_TYPES.CallExpression &&
node.parent.property.name === 'not';

export enum DescribeAlias {
'describe' = 'describe',
'fdescribe' = 'fdescribe',
Expand Down