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

Create rule schema to support "allowComments" #87

Merged
merged 2 commits into from
May 26, 2024
Merged

Conversation

azeemba
Copy link
Owner

@azeemba azeemba commented May 19, 2024

eslint v9 made schema mandatory for options supported by rules:
https://eslint.org/docs/latest/extend/custom-rules#options-schemas

Previously we didn't use the meta object on the rules so now we create the meta object and populate the schema property.

Thanks to @BePo65 for providing context, docs and the schema itself.

eslint v9 made schema mandatory for options supported
by rules:
https://eslint.org/docs/latest/extend/custom-rules#options-schemas

Previously we didn't use the meta object on the rules
so now we create the meta object and populate the `schema` property.

Thanks to @BePo65 for providing context, docs and the schema itself.
@azeemba
Copy link
Owner Author

azeemba commented May 19, 2024

@Standard8 and @BePo65 let me know if you have any thoughts on this PR.

I plan to land it during the week and push a new major release next weekend.

@BePo65
Copy link
Contributor

BePo65 commented May 21, 2024

The pr looks good. Only test coverage for lines 58-62 and 150-154 is missing.

So I am trying to add 1 or more tests for the allowComments setting.
The problem is that here we do not test rules, but a configuration setting. I'm trying to find out, how to add configuration settings for the tested rule.

Perhaps you could give me a hint, where in the unit.test.js file config settings can be 'sent' to the plugin; otherwise it will take me a little bit longer (I don't call me lazy, but just efficient 😄 ). Any help appreciated.

@Standard8
Copy link
Contributor

Perhaps you could give me a hint, where in the unit.test.js file config settings can be 'sent' to the plugin; otherwise it will take me a little bit longer (I don't call me lazy, but just efficient 😄 ). Any help appreciated.

I don't think there is an easy way. The unit tests appear to be testing only the processor, not from the rule level (i.e. they're not using RuleTester).

However, I would note that the integration tests are covering comments, it just doesn't show up on the code coverage.

@BePo65
Copy link
Contributor

BePo65 commented May 22, 2024

So perhaps it would be the best solution to add rules test (with coverage) in a new pr.

That would give me a little bit more time for this topic (at the moment I'm quite busy in my main project). But nevertheless I would like to get a deeper understanding of custom eslint rules anyway and testing the rule algorithm (with coverage) would be a good reason to do that.

@azeemba is this an acceptable way you are willing to live with?

@BePo65
Copy link
Contributor

BePo65 commented May 25, 2024

Prepared a pr (extending this pr) that adds tests for the rules to the unit tests. These tests bring coverage back to 100%.

Should we add this modification to this pr here or create a new pr (or simply drop the idea 😄 )?

@azeemba
Copy link
Owner Author

azeemba commented May 26, 2024

@BePo65 thanks for looking into it! A separate PR would be awesome.

I will go ahead and merge this PR for now and prep a release.

@azeemba azeemba merged commit 2e68d76 into master May 26, 2024
2 checks passed
@azeemba azeemba deleted the schema-for-9 branch May 26, 2024 14:02
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.

None yet

3 participants