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

valid-type: use type checking to determine test name type? #251

Closed
JoshuaKGoldberg opened this issue Sep 11, 2023 · 3 comments
Closed

valid-type: use type checking to determine test name type? #251

JoshuaKGoldberg opened this issue Sep 11, 2023 · 3 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Sep 11, 2023

I noticed that the valid-title rule doesn't use typed linting to determine whether a title is a string (or, pending #247, a function). It just goes off the AST node type. That means dynamic nodes such as Identifiers will always be detected as non-strings:

let nameBase = "My name: ";

const makeName = (suffix: string) => `first ${suffix}`;

describe(makeName("second"), () => { /* ... */ } /*
~~~~~~~~ */
// Test title must be a string.

What is your viewpoint on enabling typed linting (https://typescript-eslint.io/developers/custom-rules#typed-rules)? It would be a breaking change for existing users and have configuration (https://typescript-eslint.io/linting/typed-linting#specifying-tsconfigs / https://typescript-eslint.io/linting/typed-linting/monorepos) & performance (https://typescript-eslint.io/linting/typed-linting#how-is-performance) implications. But I think this is effectively a blocker for #247?

Note that we recommend against rules "progressively enhancing" with type information (except if there's an explicit option): typescript-eslint/typescript-eslint#7440 -> typescript-eslint/typescript-eslint#7440 (comment)

@so1ve
Copy link
Contributor

so1ve commented Oct 2, 2023

If these rules requires type information, this will become a breaking change. I prefer implementing it in a separate plugin. (Maybe eslint-plugin-vitest-typechecked? :P)

@veritem
Copy link
Owner

veritem commented Oct 3, 2023

I don't think it should be necessary a different package. I feel like if we bump another version people will stick to whatever they like. Josh, I'll look into this over fall break.

@so1ve
Copy link
Contributor

so1ve commented Oct 3, 2023

Maybe introduce a new option "typecheck"?

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 a pull request may close this issue.

3 participants