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 no-error-ctor-with-notthrows rule #326

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

GMartigny
Copy link
Contributor

@GMartigny GMartigny commented Mar 24, 2021

Fixes #178.

Continue from previous PR #240 with latest review.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


rahgurung and others added 8 commits March 24, 2021 15:58
changed name of test
and added more tests
with other requested changes.
defined variables just before
they are used and added more
tests
@sindresorhus
Copy link
Member

Since the previous PR, AVA changed its throws assertion. It now accepts an object instead, so it makes more sense to detect that: https://github.com/avajs/ava/blob/main/docs/03-assertions.md#throwsfn-expectation-message

@sindresorhus
Copy link
Member

I wonder if it now makes more sense to improve https://github.com/avajs/eslint-plugin-ava/blob/main/docs/rules/assertion-arguments.md than to add a new rule for this?

@GMartigny
Copy link
Contributor Author

I wonder if it now makes more sense to improve https://github.com/avajs/eslint-plugin-ava/blob/main/docs/rules/assertion-arguments.md than to add a new rule for this?

IMO, this rule affect all assertion. Using it only for .throws or .notThrows is not optimal.

It now accepts an object instead, so it makes more sense to detect that.

What do you mean ? Should the rule enforce the use of expectation object or disallow the use of constructor in notThrows. IMO, these are two distinct rules, so the first one can be addressed in another PR.

@sindresorhus
Copy link
Member

IMO, this rule affect all assertion. Using it only for .throws or .notThrows is not optimal.

Why is it not optimal?

Should the rule enforce the use of expectation object or disallow the use of constructor in notThrows.

Enforce expectation object. It no longer makes any sense to disallow a constructor as the user wouldn't use a constructor directly for t.throws, so they wouldn't mistakingly use it in t.notThrows.

@GMartigny
Copy link
Contributor Author

Why is it not optimal?

I feel like having a rule applying to all assertion, and sometimes another one is kinda misleading. But it might not be a big deal. You decision in the end.

@sindresorhus
Copy link
Member

I feel like having a rule applying to all assertion, and sometimes another one is kinda misleading.

Not sure how that is misleading. It would be documented. It's not like the user wouldn't want this either as it catches a bug that will crash at runtime. I think assertion-arguments is a good fit for this.

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.

Prevent specifying error type in t.notThrows()
3 participants