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

Adds jest/no-if #232

Merged
merged 3 commits into from
Mar 27, 2019
Merged

Adds jest/no-if #232

merged 3 commits into from
Mar 27, 2019

Conversation

cartogram
Copy link
Contributor

Closes #227

@cartogram cartogram force-pushed the no-if branch 2 times, most recently from 310ed4a to bdf93cd Compare March 22, 2019 03:40
docs/rules/jest/no-if.md Outdated Show resolved Hide resolved
lib/rules/jest/no-if.js Outdated Show resolved Hide resolved
'fit',
'test',
'xtest',
'describe',
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure you need to include describe blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured I'd leave it cause it's not like we want to allow:

describe('SomeThing', () => {
  if(!someFlag) {
     it('testing', () => {
     //...
     })
  }
})

}

context.report({
messageId: 'noIf',
Copy link
Member

Choose a reason for hiding this comment

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

TIL

if (notTestFunction(node) || hasEmptyBody(node)) {
return;
}
inCallExpression = true;
Copy link
Member

Choose a reason for hiding this comment

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

The iffy thing with doing this in scope is that nesting can throw things off. Nesting might not be too much of an issue here but you still might want to go with the more formal mechanism for this (state)

Copy link
Contributor Author

@cartogram cartogram Mar 25, 2019

Choose a reason for hiding this comment

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

Can you clarify what you mean when you say more formal mechanism for this (state)?

Fixing the nesting part should be easy if I just keep track of the depth instead of just a boolean for being in a call expression or not. Maybe that is what you meant by state?

Copy link
Member

Choose a reason for hiding this comment

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

I meant having it on the context.state or whatever that field is called so that it is scoped to the part of the tree where you are actually inside the test block (structurally, I don't think it makes much of a difference, but that's what the feature is there for)

Copy link
Member

Choose a reason for hiding this comment

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

Wait actually maybe I am getting mixed up with Babel plugins :P

Copy link
Contributor Author

@cartogram cartogram Mar 26, 2019

Choose a reason for hiding this comment

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

lol you are. I only know because I did the same thing. 😛

lib/rules/jest/no-if.js Outdated Show resolved Hide resolved
version "1.2.0"
resolved "https://registry.yarnpkg.com/merge/-/merge-1.2.0.tgz#7531e39d4949c281a66b8c5a6e0265e8b05894da"
integrity sha1-dTHjnUlJwoGma4xabgJl6LBYlNo=
merge@1.2.1:
Copy link
Member

@lemonmade lemonmade Mar 25, 2019

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here I guess: d45d8d0

Copy link
Member

@lemonmade lemonmade left a comment

Choose a reason for hiding this comment

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

👍

@cartogram cartogram merged commit b605e43 into master Mar 27, 2019
@cartogram cartogram deleted the no-if branch March 27, 2019 18:10
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.

2 participants