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

refactor: migrate off deprecated function-style rules in all tests #16618

Merged
merged 2 commits into from
Dec 8, 2022

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Dec 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
[ ] Add something to the core
[X] Other, please explain:

What changes did you make? (Give an overview)

This is a test-only PR that helps prepare for:

This PR only touches tests (and minor change to rule tester). It updates HUNDREDS of test cases that were using function-style rules to object-style rules. I did much of this manually but fixed a few hundred instances by writing a codemod. There were over 500 test cases needing to be updated ahead of the RFC implementation PR.

By merging these test updates first, the actual RFC implementation PR (focused on the breaking changes) linked to above will become dramatically smaller and easier to review. This will reduce the chance of ending up with substantial merge conflicts by the time the RFC breaking change PR can be merged for ESLint v9, and these test improvements are good to get in regardless. This will finally pay down the tech debt of still using hundreds of function-style rules in so many of our test cases. Updating and fixing these tests was ~6 hours of work but hopefully we're close to being done with function-style rules for good after this!

I recommend ignoring whitespace when reviewing.

Very rough jscodeshift codemod I wrote for limited parts of this (this is not at all complete, needs a lot of tweaking to handle a few different situations):

module.exports = function(fileInfo, api) {
    const j = api.jscodeshift;

    return api.jscodeshift(fileInfo.source)
        .find(j.CallExpression, {
            callee: {
                property: {
                    name: "defineRule"
                }
            }
        })
        .forEach(path => {
            if (path.value.arguments.length === 2 && path.value.arguments[1].type === "ArrowFunctionExpression") {
                path.value.arguments[1] = j.objectExpression(
                    [j.objectProperty(j.literal("create"),
                        j.functionExpression(null, [], j.blockStatement([j.returnStatement(path.value.arguments[1].body)])))
                    ]
                );
            }
        })
        .toSource();
};

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

@bmish bmish requested a review from a team as a code owner December 5, 2022 15:41
@eslint-github-bot eslint-github-bot bot added triage An ESLint team member will look at this issue soon chore This change is not user-facing labels Dec 5, 2022
@netlify
Copy link

netlify bot commented Dec 5, 2022

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit 5836042
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/63901aa7dfc3180008c73055
😎 Deploy Preview https://deploy-preview-16618--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bmish bmish added triage An ESLint team member will look at this issue soon and removed triage An ESLint team member will look at this issue soon labels Dec 5, 2022
@mdjermanovic mdjermanovic added 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 Dec 5, 2022
@mdjermanovic
Copy link
Member

We should tag this as refactor: because of the changes in RuleTester.

@bmish bmish changed the title test: adopt object-style rules in all tests refactor: adopt object-style rules in all tests Dec 5, 2022
@bmish bmish changed the title refactor: adopt object-style rules in all tests refactor: migrate off deprecated function-style rules in all tests Dec 5, 2022
@bmish
Copy link
Sponsor Member Author

bmish commented Dec 5, 2022

@mdjermanovic updated from test: to refactor:.

}
}));
linter.defineRule("test", {
create: () => ({
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, I'd prefer we use the method form throughout:

{
    create() {

    }
}

This it the form we use in all of the core rules, so I think it's preferable to keep these in the same form.

Copy link
Sponsor Member Author

@bmish bmish Dec 7, 2022

Choose a reason for hiding this comment

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

@nzakas I agree with consistency, but a few things:

  1. There are 200+ instances of create: context => ({ Program(node) { ... } });, and this format is used as a shorthand because they return an object. It's too much work to update all these by hand, so I would have to write a codemod to change to the longer form create(context) { return { Program(node) { ... } } } including adding the explicit return statement. Perhaps we should allow the shorthand style for functions that simply return an object.
  2. Based on your request, I enabled the lint rule object-shorthand: ["error", "always", { "avoidExplicitReturnArrows": true }] and auto-fixed 100+ instances of the create function to the style you prefer (in a separate commit). When we're dealing with hundreds of instances of a function call, as we are in this PR, I think it's critical that we use linting/autofixers to enforce any style preference we have, instead of relying on manual edits.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, thanks.

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. Thanks!

@nzakas
Copy link
Member

nzakas commented Dec 8, 2022

Leaving open in case @mdjermanovic also wants to review.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 90c9219 into eslint:main Dec 8, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 24, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.29.0...v8.30.0)

#### Features

-   [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#&#8203;16637](eslint/eslint#16637)) (Daniel Bartholomae)
-   [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#&#8203;16654](eslint/eslint#16654)) (Sébastien Règne)

#### Bug Fixes

-   [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#&#8203;16579](eslint/eslint#16579)) (Nicholas C. Zakas)
-   [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#&#8203;16611](eslint/eslint#16611)) (Milos Djermanovic)

#### Documentation

-   [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#&#8203;16663](eslint/eslint#16663)) (Nicholas C. Zakas)
-   [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#&#8203;16563](eslint/eslint#16563)) (Ben Perlmutter)
-   [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#&#8203;16606](eslint/eslint#16606)) (Sam Chen)
-   [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#&#8203;16631](eslint/eslint#16631)) (Percy Ma)
-   [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#&#8203;16625](eslint/eslint#16625)) (Milos Djermanovic)
-   [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#&#8203;16628](eslint/eslint#16628)) (Karl Horky)
-   [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#&#8203;16591](eslint/eslint#16591)) (Tanuj Kanti)
-   [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#&#8203;16566](eslint/eslint#16566)) (Ben Perlmutter)
-   [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#&#8203;16607](eslint/eslint#16607)) (Pavel)
-   [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#&#8203;16603](eslint/eslint#16603)) (Sam Chen)

#### Chores

-   [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.0 ([#&#8203;16675](eslint/eslint#16675)) (Milos Djermanovic)
-   [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#&#8203;14827](eslint/eslint#14827) ([#&#8203;16315](eslint/eslint#16315)) (Patrick McElhaney)
-   [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#&#8203;16664](eslint/eslint#16664)) (Milos Djermanovic)
-   [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#&#8203;16618](eslint/eslint#16618)) (Bryan Mishkin)

</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, check this box

---

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

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689
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>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 7, 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 Jun 7, 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 chore This change is not user-facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants