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

fix: add type tests #65

Merged
merged 1 commit into from
Nov 21, 2024
Merged

fix: add type tests #65

merged 1 commit into from
Nov 21, 2024

Conversation

fasttime
Copy link
Member

Prerequisites checklist

What is the purpose of this pull request?

Add type tests and fix some problems in the types.

What changes did you make? (Give an overview)

Added type tests to the CI workflow. I had to fix some minor issues in the typings of rules metadata and in the recommended config to match the signature of ESLint.Plugin. This is why this pull request is tagged fix:.

Related Issues

fixes #61

Is there anything you'd like reviewers to focus on?

@eslint-github-bot eslint-github-bot bot added the bug Something isn't working label Nov 21, 2024
Comment on lines -38 to +42
rules: {
rules: /** @type {const} */ ({
"json/no-duplicate-keys": "error",
"json/no-empty-keys": "error",
"json/no-unsafe-values": "error",
},
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is const cast syntax. I'm using this to ensure that properties in the rules object have type "error" rather than string, because string is too wide to be assigned to a severity:

    type StringSeverity = "off" | "warn" | "error";

https://github.com/eslint/eslint/blob/v9.15.0/lib/types/index.d.ts#L917

Copy link
Member

Choose a reason for hiding this comment

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

Neat trick!

@@ -9,7 +9,7 @@

export default {
meta: {
type: "problem",
type: /** @type {const} */ ("problem"),
Copy link
Member Author

Choose a reason for hiding this comment

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

This ensures that the type property has type "problem" rather than string so it matches the definition in the interface:

        type?: "problem" | "suggestion" | "layout" | undefined;

https://github.com/eslint/eslint/blob/v9.15.0/lib/types/index.d.ts#L760

@fasttime fasttime marked this pull request as ready for review November 21, 2024 09:34
Copy link
Member

@nzakas nzakas left a comment

Choose a reason for hiding this comment

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

LGTM. This is really cool. Thanks!

@nzakas nzakas merged commit a6c0bc9 into main Nov 21, 2024
16 checks passed
@nzakas nzakas deleted the test-types branch November 21, 2024 16:42
@mdjermanovic mdjermanovic mentioned this pull request Nov 22, 2024
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Request: add type tests
2 participants