search

avajs / eslint-plugin-ava

ESLint rules for AVA
https://avajs.dev
MIT License
229 stars 49 forks source link

Add `no-error-ctor-with-notthrows` rule #326

Open GMartigny opened 3 years ago

GMartigny commented 3 years ago

Fixes #178.

Continue from previous PR #240 with latest review.

IssueHunt Summary ### Referenced issues This pull request has been submitted to: - [#178: Prevent specifying error type in `t.notThrows()`](https://issuehunt.io/repos/49722492/issues/178) ---
sindresorhus commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.