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

chore: enable some rules from eslint-plugin-unicorn internally #15878

Merged
merged 2 commits into from May 17, 2022

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented May 14, 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: Enable some rules from eslint-plugin-unicorn internally / in eslint-config-eslint

What changes did you make? (Give an overview)

eslint-plugin-unicorn is a hugely popular eslint plugin (> 1M weekly downloads, > 100 contributors) with 100+ powerful rules focused especially on encouraging modern JavaScript coding practices. Disclaimer: I have contributed to this plugin and am listed as one of its maintainers.

A previous PR by @brettz9 (#15731) attempted to add most of the unicorn/recommended configuration from this plugin. This was rejected because the unicorn/recommended configuration contains many rules that could be considered too aggressive or stylistic and it was worried that frequent changes to this large configuration could become a maintenance burden.

One suggestion coming out of the previous PR was to focus on enabling a smaller set of rules, and that is what I am doing now. I have specifically chosen a small set of rules that should be minimally-controversial and clear in their benefit. In particular, the rules I have chosen are mostly focused on using more readable/efficient string/array functions, and they are all autofixable.

This enables us to get some core unicorn linting in place now, and then potentially build off of that later. If someone wants to enable additional unicorn linting, they can open separate PRs for individual consideration/discussion.

One of the rules being enabled (unicorn/prefer-string-trim-start-end) will automate the cleanup from this earlier PR:

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

@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 May 14, 2022
@netlify
Copy link

netlify bot commented May 14, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 0e37d15
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62805a3294bc3800099cfb1f

Copy link
Contributor

@brettz9 brettz9 left a comment

Choose a reason for hiding this comment

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

LGTM. Just one slight related improvement.

I think the rules such as on preferring simpler array methods make for cleaner code, and I wouldn't expect newcomers to have a hard time understanding the whys and wherefores of the rule (and they have auto-fixers).

The preferring Set rule comes at the cost of a little more verbosity, but IIRC, we noticed some significant performance improvements in eslint-plugin-jsdoc upon making such changes.

tests/lib/rules/no-invalid-this.js Outdated Show resolved Hide resolved
Co-authored-by: Brett Zamir <brettz9@yahoo.com>
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.

Given that these are fixable, I’m ok with it.

Copy link
Contributor

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM.

@snitin315 snitin315 merged commit c7894cd into eslint:main May 17, 2022
@bmish bmish 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 May 17, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request May 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.15.0` -> `8.16.0`](https://renovatebot.com/diffs/npm/eslint/8.15.0/8.16.0) |

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.15.0...v8.16.0)

#### Features

-   [`cab0c22`](eslint/eslint@cab0c22) feat: add Unicode flag suggestion in no-misleading-character-class ([#&#8203;15867](eslint/eslint#15867)) (Milos Djermanovic)
-   [`38ae956`](eslint/eslint@38ae956) feat: check Unicode code point escapes in no-control-regex ([#&#8203;15862](eslint/eslint#15862)) (Milos Djermanovic)
-   [`ee69cd3`](eslint/eslint@ee69cd3) feat: Update global variables ([#&#8203;15871](eslint/eslint#15871)) (Sébastien Règne)

#### Bug Fixes

-   [`3f09aab`](eslint/eslint@3f09aab) fix: function-paren-newline crash on "new new Foo();" ([#&#8203;15850](eslint/eslint#15850)) (coderaiser)

#### Documentation

-   [`050d5f4`](eslint/eslint@050d5f4) docs: Static further reading links ([#&#8203;15890](eslint/eslint#15890)) (Nicholas C. Zakas)
-   [`36287c0`](eslint/eslint@36287c0) docs: fix absolute paths in related rules shortcode to work from /docs ([#&#8203;15892](eslint/eslint#15892)) (Milos Djermanovic)
-   [`90b6990`](eslint/eslint@90b6990) docs: fix absolute links in rule macro to work from /docs ([#&#8203;15891](eslint/eslint#15891)) (Milos Djermanovic)
-   [`f437249`](eslint/eslint@f437249) docs: Adjust docs site path prefix ([#&#8203;15889](eslint/eslint#15889)) (Nicholas C. Zakas)
-   [`6e16025`](eslint/eslint@6e16025) docs: update 'Related Rules' and 'Further Reading' in remaining rules ([#&#8203;15884](eslint/eslint#15884)) (Milos Djermanovic)
-   [`1d39f69`](eslint/eslint@1d39f69) docs: remove confusing examples for no-mixed-operators ([#&#8203;15875](eslint/eslint#15875)) (Milos Djermanovic)
-   [`3071d76`](eslint/eslint@3071d76) docs: Fix some grammar issues ([#&#8203;15837](eslint/eslint#15837)) (byodian)

#### Chores

-   [`1768d0d`](eslint/eslint@1768d0d) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).3.0 ([#&#8203;15903](eslint/eslint#15903)) (Milos Djermanovic)
-   [`c686e4c`](eslint/eslint@c686e4c) chore: Add deploy workflow for docs site ([#&#8203;15894](eslint/eslint#15894)) (Nicholas C. Zakas)
-   [`c7894cd`](eslint/eslint@c7894cd) chore: enable some rules from eslint-plugin-unicorn internally ([#&#8203;15878](eslint/eslint#15878)) (Bryan Mishkin)
-   [`ea65cb5`](eslint/eslint@ea65cb5) chore: upgrade eslint-plugin-eslint-plugin@^4.2.0 ([#&#8203;15882](eslint/eslint#15882)) (唯然)
-   [`cc29c69`](eslint/eslint@cc29c69) chore: Upgrade official GitHub actions to latest versions ([#&#8203;15880](eslint/eslint#15880)) (Darius Dzien)
-   [`5891c75`](eslint/eslint@5891c75) chore: Refactor rule docs format ([#&#8203;15869](eslint/eslint#15869)) (Nicholas C. Zakas)

</details>

---

### Configuration

📅 **Schedule**: 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).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1366
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
srijan-deepsource pushed a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
…t#15878)

* chore: add eslint-plugin-unicorn internally

* Update tests/lib/rules/no-invalid-this.js

Co-authored-by: Brett Zamir <brettz9@yahoo.com>

Co-authored-by: Brett Zamir <brettz9@yahoo.com>
srijan-deepsource added a commit to DeepSourceCorp/eslint that referenced this pull request May 30, 2022
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Nov 14, 2022
@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 Nov 14, 2022
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

6 participants