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 deprecation warnings for legacy API in RuleTester #16063

Merged
merged 10 commits into from Jul 28, 2022

Conversation

snitin315
Copy link
Contributor

@snitin315 snitin315 commented Jun 28, 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)

Resolve #15761
Refers #14709

Prepares for upcoming breaking changes in ESLint v9 to disallow function-style rules 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?

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");

I'm not sure if we should add the above line in the deprecation message. Two links in the log can be overwhelming. Maybe we can mention this somewhere in our docs here - https://eslint.org/docs/latest/developer-guide/working-with-rules

@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon feature This change adds a new feature to ESLint labels Jun 28, 2022
@netlify
Copy link

netlify bot commented Jun 28, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 9bdb8fa
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62db6dbc1666a9000833ee90

@snitin315 snitin315 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 Jun 28, 2022
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
tests/lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
tests/lib/rule-tester/rule-tester.js Show resolved Hide resolved
@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 4, 2022

I checked all tests that are using function-style fixture rules, and it seems that only this one is specifically testing how RuleTester handles function-style rules, so I think we should convert all other fixture rules to object-style now. That will avoid logging deprecation warnings when we run npm test, and also ensure that RuleTester functionalities are tested with object-style rules, which seems more relevant than testing with legacy function-style rules.

Those fixure rules are:

@snitin315
Copy link
Contributor Author

snitin315 commented Jul 4, 2022

@mdjermanovic Thank you for investigating this, I'll update the PR with your suggestions.

@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 4, 2022

only this one is specifically testing how RuleTester handles function-style rules

As for this one, I think we can just remove that test case + two test cases right before it; in total, we can now remove these 3 test cases.

These 3 test cases were added to check RuleTester-only errors we introduced in v7.6.0 (ecb2b73) as a notice about future Linter errors that were planned and eventually implemented in v8.0.0 (4a7aab7). Since these are now Linter errors, there's no need to test them in RuleTester.

@snitin315 snitin315 force-pushed the feat/add-deprecation-warning branch from c7a8100 to 967ffb9 Compare Jul 5, 2022
tests/lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
lib/rule-tester/rule-tester.js Outdated Show resolved Hide resolved
@snitin315 snitin315 force-pushed the feat/add-deprecation-warning branch from 847ee3b to 67f88ac Compare Jul 9, 2022
@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 19, 2022

ESLint currently allows schema as a top-level property in object-style rules, and it's treated the same as meta.schema.

// rule
module.exports = {

    // this will be treated as the options schema
    schema: {
        /* ... */
    },

    create(context) {
        /* ... */
    }
};

This is most likely an oversight. getRuleOptionsSchema gets schema from the given rule by rule.schema || rule.meta && rule.meta.schema. I believe rule.schema was only intended to get schema from function-style rules, but it works for object-style rules as well.

This PR does not use getRuleOptionsSchema (it gets the schema directly from rule.meta.schema) so it will log the warning as if the schema is not defined. I think this behavior is correct and that we want to drop support for top-level schema properties in favor of meta.schema, but this wasn't mentioned in the RFC. @eslint/eslint-tsc thoughts?

@nzakas
Copy link
Member

nzakas commented Jul 20, 2022

I think this behavior is correct and that we want to drop support for top-level schema properties in favor of meta.schema, but this wasn't mentioned in the RFC.

I agree. 👍

@bmish
Copy link
Sponsor Member

bmish commented Jul 22, 2022

Good catch, I'd very much like to drop support for top-level schema in this coming major release given that it wasn't intended nor documented for object-style rules.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@nzakas
Copy link
Member

nzakas commented Jul 22, 2022

Should we also duplicate these changes in FlatRuleTester?

@bmish
Copy link
Sponsor Member

bmish commented Jul 23, 2022

@nzakas good point, it seems like we have to duplicate these changes/tests in FlatRuleTester, otherwise we'll have divergence and these changes will be lost.

@mdjermanovic
Copy link
Member

mdjermanovic commented Jul 23, 2022

Should we also duplicate these changes in FlatRuleTester?

I think it isn't necessary since we are planning eslint/rfcs#85 for ESLint v9, and FlatRuleTester will not replace RuleTester before ESLint v9. When we implement eslint/rfcs#85 in linter/config, these deprecation warnings will become errors thrown by linter/config.

bmish
bmish approved these changes Jul 27, 2022
Copy link
Sponsor Member

@bmish bmish left a comment

Thanks for your work on this!

@nzakas
Copy link
Member

nzakas commented Jul 28, 2022

Fair enough.

@nzakas nzakas merged commit 1cdcbca into main Jul 28, 2022
20 checks passed
@nzakas nzakas deleted the feat/add-deprecation-warning branch Jul 28, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Aug 9, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.20.0` -> `8.21.0`](https://renovatebot.com/diffs/npm/eslint/8.20.0/8.21.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.21.0`](https://github.com/eslint/eslint/releases/tag/v8.21.0)

[Compare Source](eslint/eslint@v8.20.0...v8.21.0)

#### Features

-   [`7b43ea1`](eslint/eslint@7b43ea1) feat: Implement FlatESLint ([#&#8203;16149](eslint/eslint#16149)) (Nicholas C. Zakas)
-   [`92bf49a`](eslint/eslint@92bf49a) feat: improve the key width calculation in `key-spacing` rule ([#&#8203;16154](eslint/eslint#16154)) (Nitin Kumar)
-   [`c461542`](eslint/eslint@c461542) feat: add new `allowLineSeparatedGroups` option to the `sort-keys` rule ([#&#8203;16138](eslint/eslint#16138)) (Nitin Kumar)
-   [`1cdcbca`](eslint/eslint@1cdcbca) feat: add deprecation warnings for legacy API in `RuleTester` ([#&#8203;16063](eslint/eslint#16063)) (Nitin Kumar)

#### Bug Fixes

-   [`0396775`](eslint/eslint@0396775) fix: lines-around-comment apply `allowBlockStart` for switch statements ([#&#8203;16153](eslint/eslint#16153)) (Nitin Kumar)

#### Documentation

-   [`2aadc93`](eslint/eslint@2aadc93) docs: add anchors to headings inside docs content ([#&#8203;16134](eslint/eslint#16134)) (Strek)

#### Chores

-   [`8892511`](eslint/eslint@8892511) chore: Upgrade to Espree 9.3.3 ([#&#8203;16173](eslint/eslint#16173)) (Brandon Mills)
-   [`1233bee`](eslint/eslint@1233bee) chore: switch to eslint-plugin-node's maintained fork ([#&#8203;16150](eslint/eslint#16150)) (唯然)
-   [`97b95c0`](eslint/eslint@97b95c0) chore: upgrade puppeteer v13 ([#&#8203;16151](eslint/eslint#16151)) (唯然)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMzUuMSIsInVwZGF0ZWRJblZlciI6IjMyLjEzNS4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1483
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion 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

5 participants