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 options to check destructuring in no-underscore-dangle #16006

Merged
merged 14 commits into from
Dec 21, 2022
Merged

feat: add options to check destructuring in no-underscore-dangle #16006

merged 14 commits into from
Dec 21, 2022

Conversation

Kaltoft
Copy link
Contributor

@Kaltoft Kaltoft commented Jun 16, 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)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?
no-underscore-dangle

Does this change cause the rule to produce more or fewer warnings?
More warnings

How will the change be implemented? (New option, new default behavior, etc.)?
New option

Please provide some example code that this change will affect:

const [foo, _bar] = list;
const [foo, ..._rest] = list;
const [foo, [bar_, baz]] = list;
const { foo, bar: _bar } = collection;
const { foo, bar, _baz } = collection;

What does the rule currently do for this code?
Currently variable names assigned from destructuring arrays and objects are not handled

What will the rule do after it's changed?
Handles variable names assigned from destructuring arrays and objects

What changes did you make? (Give an overview)

  • Added an optional check variable names from array destructuring toggled by { allowInArrayDestructuring: true }
  • Added an optional check variable names from object destructuring toggled by { allowInObjectDestructuring: true }

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

Variables assigned from deep destructuring and the ...rest pattern

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jun 16, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 16, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label Jun 16, 2022
@netlify
Copy link

netlify bot commented Jun 16, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 08c8030
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/62f10d33f598020008a47761

@btmills btmills added rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Jul 2, 2022
@btmills btmills added this to Evaluating in Triage Jul 2, 2022
@btmills
Copy link
Member

btmills commented Jul 2, 2022

Thanks for catching this omission, @Kaltoft! I'm 👍 to fixing this.

It looks like no-underscore-dangle doesn't check object destructuring either (demo). Would you be up for adding that as well?

As-is, this is a breaking change because it reports new errors on existing code. Usually, when we add additional checks to rules, we create an option as you've done here, but we default it to match the existing behavior. Then we can toggle the default to on in the next major release if we think it's something that fits with the theme of the rule.

Questions for the team to move forward:

  1. Should no-underscore-dangle check array destructuring patterns? 👍 from me.
  2. Should it also check object destructuring patterns? 👍 from me.
  3. If we say yes to both, should we have one option (currently allowDestructured: true) or two (e.g. allowInArrayDestructuring: true and allowInObjectDestructuring: true)? I'd say two separate options.

@Kaltoft
Copy link
Contributor Author

Kaltoft commented Jul 7, 2022

It looks like no-underscore-dangle doesn't check object destructuring either (demo). Would you be up for adding that as well?

Good point. I'll take a look at it.

If we say yes to both, should we have one option (currently allowDestructured: true) or two (e.g. allowInArrayDestructuring: true and allowInObjectDestructuring: true)? I'd say two separate options.

Personally I think splitting them into separate options is preferred since the use-case for destructured names in arrays is often quite different to that in objects.

@Kaltoft
Copy link
Contributor Author

Kaltoft commented Jul 7, 2022

I've updated the PR to check for variables names from object destructuring as well.

The default value for both options are now set to true, following @btmills advice and to not introduce any breaking changes. My suggestion would be to set the default to false for next major release, as I see this as expected behaviour.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes! The implementation and existing tests look good. In addition to my two comments on the diff, will you please sign our CLA?

docs/src/rules/no-underscore-dangle.md Show resolved Hide resolved
tests/lib/rules/no-underscore-dangle.js Show resolved Hide resolved
@Kaltoft
Copy link
Contributor Author

Kaltoft commented Jul 11, 2022

CLA signed and all tests passing 👌

@Kaltoft Kaltoft requested a review from btmills July 22, 2022 12:01
lib/rules/no-underscore-dangle.js Outdated Show resolved Hide resolved
lib/rules/no-underscore-dangle.js Outdated Show resolved Hide resolved
lib/rules/no-underscore-dangle.js Outdated Show resolved Hide resolved
@Kaltoft
Copy link
Contributor Author

Kaltoft commented Aug 8, 2022

Thank you @mdjermanovic I've updated the PR to address the issues you mentioned.

@github-actions
Copy link

Oops! It looks like we lost track of this pull request. What do we want to do here? This pull request will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 14, 2022
@Kaltoft
Copy link
Contributor Author

Kaltoft commented Oct 17, 2022

Oops! It looks like we lost track of this pull request. What do we want to do here? This pull request will auto-close in 7 days without an update.

@mdjermanovic @btmills could you check this again? Everything should be ready

@github-actions github-actions bot removed the Stale label Oct 17, 2022
@mdjermanovic mdjermanovic changed the title feat: disallow destructured variable names feat: add options to check destructuring in no-underscore-dangle Dec 21, 2022
@mdjermanovic
Copy link
Member

I'll close & reopen just to re-run checks.

Triage automation moved this from Evaluating to Complete Dec 21, 2022
@mdjermanovic mdjermanovic reopened this Dec 21, 2022
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.

@Kaltoft apologies for the long delay in reviewing this PR.

LGTM, thanks for contributing!

@mdjermanovic mdjermanovic merged commit b401cde into eslint:main Dec 21, 2022
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Dec 21, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jan 3, 2023
This PR contains the following updates:

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

---

### Release Notes

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

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

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

#### Features

-   [`52c7c73`](eslint/eslint@52c7c73) feat: check assignment patterns in no-underscore-dangle ([#&#8203;16693](eslint/eslint#16693)) (Milos Djermanovic)
-   [`b401cde`](eslint/eslint@b401cde) feat: add options to check destructuring in no-underscore-dangle ([#&#8203;16006](eslint/eslint#16006)) (Morten Kaltoft)
-   [`30d0daf`](eslint/eslint@30d0daf) feat: group properties with values in parentheses in `key-spacing` ([#&#8203;16677](eslint/eslint#16677)) (Francesco Trotta)

#### Bug Fixes

-   [`35439f1`](eslint/eslint@35439f1) fix: correct syntax error in `prefer-arrow-callback` autofix ([#&#8203;16722](eslint/eslint#16722)) (Francesco Trotta)
-   [`87b2470`](eslint/eslint@87b2470) fix: new instance of FlatESLint should load latest config file version ([#&#8203;16608](eslint/eslint#16608)) (Milos Djermanovic)

#### Documentation

-   [`4339dc4`](eslint/eslint@4339dc4) docs: Update README (GitHub Actions Bot)
-   [`4e4049c`](eslint/eslint@4e4049c) docs: optimize code block structure ([#&#8203;16669](eslint/eslint#16669)) (Sam Chen)
-   [`54a7ade`](eslint/eslint@54a7ade) docs: do not escape code blocks of formatters examples ([#&#8203;16719](eslint/eslint#16719)) (Sam Chen)
-   [`e5ecfef`](eslint/eslint@e5ecfef) docs: Add function call example for no-undefined ([#&#8203;16712](eslint/eslint#16712)) (Elliot Huffman)
-   [`a3262f0`](eslint/eslint@a3262f0) docs: Add mastodon link ([#&#8203;16638](eslint/eslint#16638)) (Amaresh  S M)
-   [`a14ccf9`](eslint/eslint@a14ccf9) docs: clarify files property ([#&#8203;16709](eslint/eslint#16709)) (Sam Chen)
-   [`3b29eb1`](eslint/eslint@3b29eb1) docs: fix npm link ([#&#8203;16710](eslint/eslint#16710)) (Abdullah Osama)
-   [`a638673`](eslint/eslint@a638673) docs: fix search bar focus on `Esc` ([#&#8203;16700](eslint/eslint#16700)) (Shanmughapriyan S)
-   [`f62b722`](eslint/eslint@f62b722) docs: country flag missing in windows ([#&#8203;16698](eslint/eslint#16698)) (Shanmughapriyan S)
-   [`4d27ec6`](eslint/eslint@4d27ec6) docs: display zh-hans in the docs language switcher ([#&#8203;16686](eslint/eslint#16686)) (Percy Ma)
-   [`8bda20e`](eslint/eslint@8bda20e) docs: remove manually maintained anchors ([#&#8203;16685](eslint/eslint#16685)) (Percy Ma)
-   [`b68440f`](eslint/eslint@b68440f) docs: User Guide Getting Started expansion ([#&#8203;16596](eslint/eslint#16596)) (Ben Perlmutter)

#### Chores

-   [`65d4e24`](eslint/eslint@65d4e24) chore: Upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.1 ([#&#8203;16729](eslint/eslint#16729)) (Brandon Mills)
-   [`8d93081`](eslint/eslint@8d93081) chore: fix CI failure ([#&#8203;16721](eslint/eslint#16721)) (Sam Chen)
-   [`8f17247`](eslint/eslint@8f17247) chore: Set up automatic updating of README ([#&#8203;16717](eslint/eslint#16717)) (Nicholas C. Zakas)
-   [`4cd87cb`](eslint/eslint@4cd87cb) ci: bump actions/stale from 6 to 7 ([#&#8203;16713](eslint/eslint#16713)) (dependabot\[bot])
-   [`fd20c75`](eslint/eslint@fd20c75) chore: sort package.json scripts in alphabetical order ([#&#8203;16705](eslint/eslint#16705)) (Darius Dzien)
-   [`10a5c78`](eslint/eslint@10a5c78) chore: update ignore patterns in `eslint.config.js` ([#&#8203;16678](eslint/eslint#16678)) (Milos Djermanovic)

</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:eyJjcmVhdGVkSW5WZXIiOiIzNC43Ni4yIiwidXBkYXRlZEluVmVyIjoiMzQuNzYuMiJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1697
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>
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 19, 2023
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 25, 2023
* docs: User Guide Getting Started expansion

* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: adjust some words

* docs: Add function call example for no-undefined

eslint/eslint#16712

* docs: check assignment patterns in no-underscore-dangle

eslint/eslint#16693

* update formatters

* Apply suggestions from code review

Co-authored-by: Strek <ssharishkumar@gmail.com>

---------

Co-authored-by: Strek <ssharishkumar@gmail.com>
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
* docs: User Guide Getting Started expansion

* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: adjust some words

* docs: Add function call example for no-undefined

eslint/eslint#16712

* docs: check assignment patterns in no-underscore-dangle

eslint/eslint#16693

* update formatters

* Apply suggestions from code review

Co-authored-by: Strek <ssharishkumar@gmail.com>

---------

Co-authored-by: Strek <ssharishkumar@gmail.com>
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
* docs: User Guide Getting Started expansion

* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: adjust some words

* docs: Add function call example for no-undefined

eslint/eslint#16712

* docs: check assignment patterns in no-underscore-dangle

eslint/eslint#16693

* update formatters

* Apply suggestions from code review

Co-authored-by: Strek <ssharishkumar@gmail.com>

---------

Co-authored-by: Strek <ssharishkumar@gmail.com>
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
kecrily added a commit to eslint/zh-hans.docs.eslint.org that referenced this pull request Feb 28, 2023
* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: check assignment patterns in no-underscore-dangle

eslint/eslint#16693

* docs: use inline code for rule options name

eslint/eslint#16768

* docs: ignore directives for no-fallthrough

eslint/eslint#16757

* docs: IA Update page URL move

eslint/eslint#16665

* update

* feat: sync v8.31.0 (#94)

* docs: User Guide Getting Started expansion

* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: adjust some words

* docs: Add function call example for no-undefined

eslint/eslint#16712

* docs: check assignment patterns in no-underscore-dangle

eslint/eslint#16693

* update formatters

* Apply suggestions from code review

Co-authored-by: Strek <ssharishkumar@gmail.com>

---------

Co-authored-by: Strek <ssharishkumar@gmail.com>

* docs: add options to check destructuring in no-underscore-dangle

eslint/eslint#16006

* docs: IA Update page URL move

eslint/eslint#16665

---------

Co-authored-by: Strek <ssharishkumar@gmail.com>
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 20, 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 20, 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 contributor pool feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

4 participants