Skip to content

Chore: update no-unused-vars caughtErrors in eslint-config-eslint#13351

Merged
kaicataldo merged 1 commit intomasterfrom
eslintconfigeslint-caughterrors
Jun 1, 2020
Merged

Chore: update no-unused-vars caughtErrors in eslint-config-eslint#13351
kaicataldo merged 1 commit intomasterfrom
eslintconfigeslint-caughterrors

Conversation

@mdjermanovic
Copy link
Copy Markdown
Member

@mdjermanovic mdjermanovic commented May 24, 2020 •
edited
Loading

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[X] Other, please explain:

Updates eslint-config-eslint

What changes did you make? (Give an overview)

Added caughtErrors: "all" to no-unused-vars in order to enforce omission of catch binding in cases where the caught error would not be used.

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

Does this change make sense? Also, please verify that all supported Node versions support optional catch binding.

@mdjermanovic mdjermanovic added the chore This change is not user-facing label May 24, 2020
@aladdin-add
Copy link
Copy Markdown
Member

aladdin-add commented May 25, 2020

image

the supported Node versions seems fine.(and the CI passed)

@yeonjuan
Copy link
Copy Markdown
Member

yeonjuan commented May 28, 2020 •
edited
Loading

@mdjermanovic
I test it in node v10.12.0 and it looks fine.
Maybe should we update the engine field in eslint-config-eslint package.json?
It's still "node": "^8.10.0 || ^10.13.0 || >=11.10.1"

@kaicataldo
Copy link
Copy Markdown
Member

kaicataldo commented May 28, 2020

Even though it won't cause any issues, I think it does makes sense to have it match the engines range in eslint's package.json.

@mdjermanovic
Copy link
Copy Markdown
Member Author

mdjermanovic commented Jun 1, 2020

Maybe should we update the engine field in eslint-config-eslint package.json?
It's still "node": "^8.10.0 || ^10.13.0 || >=11.10.1"

Should we update the engine field in this PR, because the change made so far in this PR basically requires catch {}, which isn't supported in 8.10.0?

@yeonjuan
Copy link
Copy Markdown
Member

yeonjuan commented Jun 1, 2020

Should we update the engine field in this PR, because the change made so far in this PR basically requires catch {}, which isn't supported in 8.10.0?

IMO, it's not strongly necessary for this PR. 😀

yeonjuan
yeonjuan approved these changes Jun 1, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@yeonjuan yeonjuan 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 :)

@aladdin-add
Copy link
Copy Markdown
Member

aladdin-add commented Jun 1, 2020

well, I've created a PR #13379 to update it.

@kaicataldo kaicataldo merged commit 578efad into master Jun 1, 2020
@kaicataldo kaicataldo deleted the eslintconfigeslint-caughterrors branch June 1, 2020 19:05
@eslint-deprecated eslint-deprecated Bot locked and limited conversation to collaborators Nov 29, 2020
@eslint-deprecated eslint-deprecated Bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@kaicataldo kaicataldo kaicataldo approved these changes

@yeonjuan yeonjuan yeonjuan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mdjermanovic @aladdin-add @yeonjuan @kaicataldo