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

Update: add `ignoreDestructuring` option to `id-match` rule #10554

Merged
merged 1 commit into from Nov 9, 2018

Conversation

@tinymins
Copy link
Contributor

commented Jul 4, 2018

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

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

add ignoreDestructuring option to id-match rule

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

Nope.

Sorry that I do this code split job so late, cause I caught a cold and fever last week.

@platinumazure
Copy link
Member

left a comment

Thanks for the PR!

One question: What is the current behavior of renamed and non-renamed import specifiers, and the intended behavior of the same after this pull request? (I'm wondering if we should have another option for import specifiers that works similarly for renamed import specifiers, maybe as a separate PR.)

@tinymins tinymins force-pushed the tinymins:id-match-ignore-destructuring branch from 66eee77 to dff1a1c Jul 5, 2018

@tinymins

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2018

@platinumazure I've recommitted, would you mind review again to see if there's any more problems should be fixed? Thanks a lot! :-)

@tinymins tinymins force-pushed the tinymins:id-match-ignore-destructuring branch 3 times, most recently from 202ac79 to 0001128 Jul 5, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Sep 1, 2018

Hi @tinymins, very sorry for losing track of this.

I'll champion this issue. (Will hopefully work with the ESLint team to accept this issue soon!)

I'll review this over the weekend!

@platinumazure platinumazure self-assigned this Sep 1, 2018

@tinymins

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@platinumazure Thanks, with any problem in this PR, just point it out here, I'll check Github email everyday.

lib/rules/id-match.js Outdated Show resolved Hide resolved
tests/lib/rules/id-match.js Outdated Show resolved Hide resolved
lib/rules/id-match.js Show resolved Hide resolved
lib/rules/id-match.js Outdated Show resolved Hide resolved
docs/rules/id-match.md Show resolved Hide resolved

@tinymins tinymins force-pushed the tinymins:id-match-ignore-destructuring branch 2 times, most recently from 73a390d to 68e7f95 Oct 6, 2018

@aladdin-add
Copy link
Member

left a comment

LGTM, thanks! just need one more 👍 to accept it. /cc @eslint/eslint-team

lib/rules/id-match.js Outdated Show resolved Hide resolved
lib/rules/id-match.js Outdated Show resolved Hide resolved
lib/rules/id-match.js Outdated Show resolved Hide resolved

@tinymins tinymins force-pushed the tinymins:id-match-ignore-destructuring branch from 68e7f95 to 64d2e39 Oct 10, 2018

@btmills

btmills approved these changes Nov 9, 2018

Copy link
Member

left a comment

LGTM, thanks @tinymins!

@nzakas nzakas added accepted and removed evaluating labels Nov 9, 2018

@nzakas nzakas merged commit c832cd5 into eslint:master Nov 9, 2018

5 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@nzakas

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Thanks @tinymins. Sorry it took so long to merge this in, but we really appreciate your work.

@tinymins

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Thanks again for review and merge this!

@tinymins tinymins deleted the tinymins:id-match-ignore-destructuring branch Nov 14, 2018

@eslint eslint bot locked and limited conversation to collaborators May 9, 2019

@eslint eslint bot added the archived due to age label May 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.