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 rule no-sibling-hooks (fixes #82) #85

Merged
merged 5 commits into from
Aug 1, 2016

Conversation

jfmengels
Copy link
Collaborator

@jfmengels jfmengels commented Jul 26, 2016

Add rule no-sibling-hooks (fixes #82)

Let me know if you want me to change something or if I've missed something :)

I deactivatd two rules here about the function complexity. I could not make the function less complex. I could move the last part of the function to another function, but I'm not sure it would make it less complex, as you'd pass a few arguments around. Not opposed to doing that though. Let me know what you wish for me to do.

# Disallow hooks (no-hooks)

Mocha proposes hooks that allow code to be run before or after every or all tests. This helps define a common setup or teardown process for every test.
It is possible to declare a hook multiple times inside the same test suite, but it can confusing. It is better to have one hook handle the whole of the setup or teardown logic of the test suite.
Copy link
Owner

Choose a reason for hiding this comment

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

-but it can confusing
+but it can be confusing

@jfmengels
Copy link
Collaborator Author

jfmengels commented Jul 27, 2016

Additionally to that should we also support things like xdescribe, xcontext, describe.skip, describe.only?

Added support for those

Should we also support mocha’s TDD interface which uses suite instead of describe and maybe also the describe alias context?

Still TBD. I'd vote for no unless someone really wants it or it's properly handled with the other rules. If this is the only rule that will support it, I don't see the point.
EDIT: I get the feeling that it actually is. So up to you @lo1tuma to decide :)

@jfmengels
Copy link
Collaborator Author

Actually, if we want to do that, we'd probably better update no-hooks and similar (existing or upcoming rules), but I'm a bit worried that setup for instance is a too common function name, and disallowing that one in no-hooks would be an unwanted effect. It would be fine if we can know for certain whether the linted file is a test file or not, but I wouldn't know how to do that.

Or actually, we could probably do it by not reporting the variables that were declared in the file (using context.getScope(). Unless setting mocha: true in the ESLint env settings makes them available in the scope. Will try to play with this to know if we can have a nice solution.

@lo1tuma
Copy link
Owner

lo1tuma commented Aug 1, 2016

I would be ok with not supporting the TDD interface now. We can add support for it if somebody requests that.

I think the right approach would be check where setup and friends were declared (might be also imported with custom name) or if it is a global. But as I said, we can skip this for the moment and wait until somebody reports such a use-case.

@jfmengels
Copy link
Collaborator Author

I'm fine with that. Then this PR is up for review

@lo1tuma lo1tuma added the feature label Aug 1, 2016
@lo1tuma
Copy link
Owner

lo1tuma commented Aug 1, 2016

LGTM

@lo1tuma lo1tuma merged commit 55ecd79 into lo1tuma:master Aug 1, 2016
@jfmengels jfmengels deleted the no-sibling-hooks branch August 1, 2016 20:43
@jfmengels
Copy link
Collaborator Author

cheering-cheer-pom-crab-d3MM4vAejYomlzGg

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.

Rule proposal: No multiple hooks on the same level
2 participants