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

fix: Make no-unused-var function the same way in for..of loops as for..in loops #15868

Merged
merged 1 commit into from Jun 13, 2022
Merged

fix: Make no-unused-var function the same way in for..of loops as for..in loops #15868

merged 1 commit into from Jun 13, 2022

Conversation

alexbassy
Copy link
Contributor

@alexbassy alexbassy commented May 11, 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 changes did you make? (Give an overview)

Tweaked the no-unused-vars rule to treat for..of loops and for..in loops the same

Sorry for the labels 🙈 not sure if this is a bug or a feature

What rule do you want to change?

no-unused-vars`

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

Fewer

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

for..of loops will be treated the same way as for..in loops

Please provide some example code that this change will affect:

// without change
for (const name in object) { return true } // valid
for (const name of iterable) { return true } // invalid, `name` is unused

// with change
for (const name of iterable) { return true } // valid

What does the rule currently do for this code?

It will cause show a no-unused-vars error on the variable declaration in the for..of loop

What will the rule do after it's changed?

It will not show an error

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

@netlify
Copy link

netlify bot commented May 11, 2022

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit b2f6b81
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/627bdb9b8010320008d3dc79

@eslint-github-bot
Copy link

Hi @alexbassy!, 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

@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label May 11, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 11, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: alexbassy / name: Alex Bass (b2f6b81)

@eslint-github-bot eslint-github-bot bot added the feature This change adds a new feature to ESLint label May 11, 2022
@mdjermanovic mdjermanovic added this to Triaging in Triage May 12, 2022
@mdjermanovic mdjermanovic added the evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion label May 12, 2022
@mdjermanovic
Copy link
Member

Hi @alexbassy, thanks for the PR!

for (const name in object) { return true } // valid
for (const name of iterable) { return true } // invalid, `name` is unused

According to the discussion in the original issue #2342 to which this behavior relates, it seems that this pattern should have been allowed for for-of loops as well (in particular #2342 (comment)).

On the other hand, a drawback of allowing more patterns is always that some cases where the variable was intended to be used could be missed, and this pattern with for-of loops can usually be replaced with:

const [name] = iterable
if (typeof name !== "undefined") return true

Granted, this isn't fully equivalent (in particular, it doesn't account for the possibility that the first element of the sequence is undefined), so my opinion on this is neutral. I wouldn't mind including for-of if other team members agree that this is a bug (oversight in implementing accepted change #2342).

@mdjermanovic mdjermanovic moved this from Triaging to Feedback Needed in Triage May 12, 2022
@alexbassy
Copy link
Contributor Author

Thank you @mdjermanovic 🎈 for some reason, the for..of behaviour wasn’t added despite Ilya’s comment. I was also wondering why this doesn’t allow using the variable pattern ^_, which I think might address the problem you’re describing?

Happy to go with your suggestion and wait for some more feedback 🤝

@mdjermanovic
Copy link
Member

I was also wondering why this doesn’t allow using the variable pattern ^_, which I think might address the problem you’re describing?

I've just tried with varsIgnorePattern: "^_", and it seems to work (there are no errors reported): Online Demo

(maybe I didn't understand, did you mean that varsIgnorePattern doesn't allow these cases?)

@nzakas
Copy link
Member

nzakas commented Jun 11, 2022

I think it makes sense to match the behavior of for-of to that of for-in. 👍

@nzakas nzakas 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 Jun 11, 2022
@nzakas nzakas moved this from Feedback Needed to Pull Request Opened in Triage Jun 11, 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.

LGTM, thanks for contributing!

@mdjermanovic
Copy link
Member

Some checks didn't run. I'll close-reopen to see if that will trigger the missing checks.

Triage automation moved this from Pull Request Opened to Complete Jun 13, 2022
@mdjermanovic mdjermanovic reopened this Jun 13, 2022
@mdjermanovic mdjermanovic merged commit f364d47 into eslint:main Jun 13, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Jun 22, 2022
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](eslint/eslint@v8.17.0...v8.18.0)

#### Features

-   [`a6273b8`](eslint/eslint@a6273b8) feat: account for rule creation time in performance reports ([#&#8203;15982](eslint/eslint#15982)) (Nitin Kumar)

#### Bug Fixes

-   [`f364d47`](eslint/eslint@f364d47) fix: Make no-unused-vars treat for..of loops same as for..in loops ([#&#8203;15868](eslint/eslint#15868)) (Alex Bass)

#### Documentation

-   [`4871047`](eslint/eslint@4871047) docs: Update analytics, canonical URL, ads ([#&#8203;15996](eslint/eslint#15996)) (Nicholas C. Zakas)
-   [`cddad14`](eslint/eslint@cddad14) docs: Add correct/incorrect containers ([#&#8203;15998](eslint/eslint#15998)) (Nicholas C. Zakas)
-   [`b04bc6f`](eslint/eslint@b04bc6f) docs: Add rules meta info to rule pages ([#&#8203;15902](eslint/eslint#15902)) (Nicholas C. Zakas)
-   [`1324f10`](eslint/eslint@1324f10) docs: unify the wording referring to optional exception ([#&#8203;15893](eslint/eslint#15893)) (Abdelrahman Elkady)
-   [`ad54d02`](eslint/eslint@ad54d02) docs: add missing trailing slash to some internal links ([#&#8203;15991](eslint/eslint#15991)) (Milos Djermanovic)
-   [`df7768e`](eslint/eslint@df7768e) docs: Switch to version-relative URLs ([#&#8203;15978](eslint/eslint#15978)) (Nicholas C. Zakas)
-   [`21d6479`](eslint/eslint@21d6479) docs: change some absolute links to relative ([#&#8203;15970](eslint/eslint#15970)) (Milos Djermanovic)
-   [`f31216a`](eslint/eslint@f31216a) docs: Update README team and sponsors (ESLint Jenkins)

#### Build Related

-   [`ed49f15`](eslint/eslint@ed49f15) build: remove unwanted parallel and image-min for dev server ([#&#8203;15986](eslint/eslint#15986)) (Strek)

#### Chores

-   [`f6e2e63`](eslint/eslint@f6e2e63) chore: fix 'replaced by' rule list ([#&#8203;16007](eslint/eslint#16007)) (Milos Djermanovic)
-   [`d94dc84`](eslint/eslint@d94dc84) chore: remove unused deprecation warnings ([#&#8203;15994](eslint/eslint#15994)) (Francesco Trotta)
-   [`cdcf11e`](eslint/eslint@cdcf11e) chore: fix versions link ([#&#8203;15995](eslint/eslint#15995)) (Milos Djermanovic)
-   [`d2a8715`](eslint/eslint@d2a8715) chore: add trailing slash to `pathPrefix` ([#&#8203;15993](eslint/eslint#15993)) (Milos Djermanovic)
-   [`58a1bf0`](eslint/eslint@58a1bf0) chore: tweak URL rewriting for local previews ([#&#8203;15992](eslint/eslint#15992)) (Milos Djermanovic)
-   [`80404d2`](eslint/eslint@80404d2) chore: remove docs deploy workflow ([#&#8203;15984](eslint/eslint#15984)) (Nicholas C. Zakas)
-   [`71bc750`](eslint/eslint@71bc750) chore: Set permissions for GitHub actions ([#&#8203;15971](eslint/eslint#15971)) (Naveen)
-   [`90ff647`](eslint/eslint@90ff647) chore: avoid generating subdirectories for each page on new docs site ([#&#8203;15967](eslint/eslint#15967)) (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, 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/1427
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>
@nzakas
Copy link
Member

nzakas commented Jun 30, 2022

@alexbassy we'd like to pay you for this contribution, but I couldn't find an email address for you. Can you please contact us at contact (at) eslint (dot) org? Thanks.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 11, 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 Dec 11, 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 bug ESLint is working incorrectly feature This change adds a new feature to ESLint
Projects
Archived in project
Triage
Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants