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

feat: add rule test deprecation warnings: fn-style rules/missing schema #15761

Closed

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Apr 5, 2022

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[X] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Prepares for upcoming breaking changes in ESLint v9 to disallow function-style rule and rules with options that are missing a schema.

Adds the deprecation warnings mentioned in the RFC (eslint/rfcs#85).

These deprecation warnings will help alert plugin authors that they need to update their rules. They are only visible when running rule tests and not to end-users who just use the rules.

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

  1. Any suggestions on how to test these messages to stdout?
  2. Is simply replicating these in flat rule tester right for now? How can I manually test the flat rule tester? I manually tested the normal rule tester. See comments.
  3. Should I switch to process.emitWarning() (see comments)?

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Apr 5, 2022
@eslint-github-bot
Copy link

Hi @bmish!, thanks for the Pull Request

The first commit message isn't properly formatted. We ask that you update the message to match this format, as we use it to generate changelogs and automate releases.

  • The length of the commit message must be less than or equal to 72

To Fix: You can fix this problem by running git commit --amend, editing your commit message, and then running git push -f to update this pull request.

Read more about contributing to ESLint here

@bmish bmish force-pushed the deprecate-function-rule-and-missing-schemas branch from cc83a0c to 2a547ca Compare April 5, 2022 21:23
@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Apr 5, 2022
@bmish bmish changed the title feat: add rule tester deprecation warnings for function-style rules and rules missing schemas feat: add rule test deprecation warnings: fn-style rules/missing schema Apr 5, 2022
@mdjermanovic
Copy link
Member

2. Is simply replicating these in flat rule tester right for now? How can I manually test the flat rule tester? I manually tested the normal rule tester.

I think it isn't necessary to add deprecation warnings to FlatRuleTester if we are planning eslint/rfcs#85 for ESLint v9, as FlatRuleTester won't be used in production before ESLint v9,

@bmish bmish force-pushed the deprecate-function-rule-and-missing-schemas branch from 2a547ca to 763b994 Compare April 6, 2022 14:35
@bmish
Copy link
Sponsor Member Author

bmish commented Apr 6, 2022

@mdjermanovic that's fair. Should I just remove the changes to FlatRuleTester then? Or should I leave them for consistency since we will want to replace these warnings with permanent asserts in the next major version.

@mdjermanovic
Copy link
Member

since we will want to replace these warnings with permanent asserts in the next major version

Is it necessary to have these checks in RuleTester in the next major version? Per the RFC, they will be implemented in the core. If it throws anyways, it seems we don't need to duplicate checks in RuleTester.

@bmish
Copy link
Sponsor Member Author

bmish commented Apr 6, 2022

@mdjermanovic good point, removed from flat rule tester.

@mdjermanovic
Copy link
Member

  1. Any suggestions on how to test these messages to stdout?

If it helps, we have some deprecation warnings in eslintrc and the tests are here.

But there we're using process.emitWarning(). I'm not sure what the advantages are over console.warn and which way would be more appropriate for these warnings.

@bmish bmish marked this pull request as ready for review April 6, 2022 22:06
@bmish
Copy link
Sponsor Member Author

bmish commented Apr 6, 2022

I added tests for console.warn.

However, it looks like process.emitWarning() and lib/shared/deprecation-warnings.js would be a bit more of a formalized and centralized means for managing these deprecation warnings. So while my new deprecation warnings are temporary and should be removed later this year with the next major release, I could still try to switch over to those existing means if desired.

Comment on lines +586 to +587
// eslint-disable-next-line no-console -- needed for temporary deprecation warning before this is converted into an assert
console.warn("DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`: https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md");
Copy link
Member

Choose a reason for hiding this comment

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

This assumes that the warning message will be logged right after the test title, but that doesn't have to be the case.

For example, if you run mocha with -R progress, it would look like this:

[▬▬..........................................................................]DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`: https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
  [▬▬▬.........................................................................]DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`: https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`:
https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
  [▬▬▬▬........................................................................]DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`: https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
  [▬▬▬▬▬.......................................................................]DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`: https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
DEPRECATION WARNING: This test case specifies `options` but the rule is missing `meta.schema` and will stop working in ESLint v9. Please add `meta.schema`:
https://eslint.org/docs/developer-guide/working-with-rules#options-schemas This lint rule can assist with the conversion: https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/blob/main/docs/rules/require-meta-schema.md
...

The problem is that there's no info about which rule is the cause of the warnings. We could add ruleName to the message. Also, it might be good to log the warning only once per rule (not for each test case that has options)?

Comment on lines 583 to +585
if (hasOwnProperty(item, "options")) {
assert(Array.isArray(item.options), "options must be an array");
if (!rule.schema) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we shouldn't log the warning if item.options is an empty array, as that's basically the same as if no options had been passed.

@mdjermanovic
Copy link
Member

However, it looks like process.emitWarning() and lib/shared/deprecation-warnings.js would be a bit more of a formalized and centralized means for managing these deprecation warnings. So while my new deprecation warnings are temporary and should be removed later this year with the next major release, I could still try to switch over to those existing means if desired.

process.emitWarning seems to be a Node API designed for use cases like this one, so it's probably right to use it instead of console.warn.

As for lib/shared/deprecation-warnings.js, I'm not sure if it is usable for this purpose, as it looks like it was designed for warnings related to configuration files.

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features accepted There is consensus among the team that this change meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 7, 2022
@snitin315
Copy link
Contributor

@bmish are you still working on this? If not, can I take over this PR?

@bmish
Copy link
Sponsor Member Author

bmish commented Jun 13, 2022

@snitin I definitely want to see this work completed. If you want to attempt it, could you open a separate PR to do so? If I can find time to continue on my PR, I'll post here.

@bmish
Copy link
Sponsor Member Author

bmish commented Jul 6, 2022

Closing in favor of #16063.

@bmish bmish closed this Jul 6, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 3, 2023
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features feature This change adds a new feature to ESLint
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants