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: create max-nested-describe rule #845

Merged
merged 14 commits into from
Jul 21, 2021

Conversation

dominictwlee
Copy link
Contributor

Resolves #362

The idea is the same as the above issue. I changed the rule name and options to something more consistent with the max-nested-callbacks rule.

src/rules/utils.ts Outdated Show resolved Hide resolved
src/rules/max-nested-describe.ts Outdated Show resolved Hide resolved
@dominictwlee dominictwlee requested a review from G-Rath May 24, 2021 19:49
@dominictwlee
Copy link
Contributor Author

Thanks for the review @G-Rath, I've made the changes according to your comments.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Cheers, this is looking good! Just got some documentation fixes, and if you could add a few modifier & .each tests that'd be great!

docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
docs/rules/max-nested-describe.md Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/max-nested-describe.test.ts Outdated Show resolved Hide resolved
@dominictwlee dominictwlee requested a review from G-Rath May 24, 2021 20:42
@dominictwlee
Copy link
Contributor Author

My bad with the brackets! been a long day my eyes must be going fuzzy.

Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

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

Awesome! No worries about the brackets - thanks for putting up with all my nitty requests 😄

src/rules/max-nested-describe.ts Outdated Show resolved Hide resolved
@G-Rath
Copy link
Collaborator

G-Rath commented May 24, 2021

I'm now wondering if 2 is a good default max, given that max-nested-callbacks has a max of 10.

Thinking about the tests I've written, I easily have more than two nested describes, and think you would easily hit that with class/method tests:

describe('MyClass', () => {
  describe('#doSomethingWithDate', () => {
    describe('when an error happens', () => {
       ...
    });

    describe('when given a number', () => {
      it('parses it as a date', () => {
         ...
      });
    });

    describe('when the date is in the future', () => {
       ...
    });
  });
});

I'm not overly fussed about having the perfect default, but wouldn't mind hearing if @SimenB has any opinions on what a good nested level might be.

@dominictwlee
Copy link
Contributor Author

dominictwlee commented May 24, 2021

I'm now wondering if 2 is a good default max, given that max-nested-callbacks has a max of 10.

Thinking about the tests I've written, I easily have more than two nested describes, and think you would easily hit that with class/method tests:

describe('MyClass', () => {
  describe('#doSomethingWithDate', () => {
    describe('when an error happens', () => {
       ...
    });

    describe('when given a number', () => {
      it('parses it as a date', () => {
         ...
      });
    });

    describe('when the date is in the future', () => {
       ...
    });
  });
});

I'm not overly fussed about having the perfect default, but wouldn't mind hearing if @SimenB has any opinions on what a good nested level might be.

Yeah agreed. it's pretty subjective so probably depends on the respective individuals/teams.Though I do think 10 is probably a bit much for nested describes.

@dominictwlee
Copy link
Contributor Author

dominictwlee commented Jun 29, 2021

@SimenB @G-Rath Hey just following up on this. Any thoughts on the default config for nested levels?

@G-Rath
Copy link
Collaborator

G-Rath commented Jun 29, 2021

@dominictwlee sorry I've been very busy recently, both working on some core changes to eslint-plugin-jest (currently, looking at using settings for configuring rules) and with other work stuff 😅

Currently I think 5 is probably a good default, as it leaves you with 3 levels of nesting after a class describe + method describe.

@dominictwlee
Copy link
Contributor Author

@dominictwlee sorry I've been very busy recently, both working on some core changes to eslint-plugin-jest (currently, looking at using settings for configuring rules) and with other work stuff

Currently I think 5 is probably a good default, as it leaves you with 3 levels of nesting after a class describe + method describe.

@G-Rath I've updated the default max to 5, and added a few tests + examples in docs for this.

@dominictwlee
Copy link
Contributor Author

@G-Rath Just following up on this periodically, let me know if anything needs to change.

@G-Rath
Copy link
Collaborator

G-Rath commented Jul 21, 2021

@dominictwlee cheers, lets land it

@G-Rath G-Rath merged commit 8067405 into jest-community:main Jul 21, 2021
github-actions bot pushed a commit that referenced this pull request Jul 21, 2021
# [24.4.0](v24.3.7...v24.4.0) (2021-07-21)

### Features

* create `max-nested-describe` rule ([#845](#845)) ([8067405](8067405))
@github-actions
Copy link

🎉 This PR is included in version 24.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dominictwlee dominictwlee deleted the max-nested-describe-rule branch July 22, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support max-describe-nesting-level rule.
2 participants