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

New: allowAfterThisConstructor for no-underscore-dangle (fixes #11488) #11489

Merged
merged 1 commit into from Oct 29, 2019

Conversation

@sripberger
Copy link
Contributor

sripberger commented Mar 7, 2019

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)

Implemented a new option for no_underscore_dangle rule

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

See rationale in issue.

@eslint eslint bot added the triage label Mar 7, 2019
@ilyavolodin
Copy link
Member

ilyavolodin commented May 25, 2019

@eslint/eslint-team does anyone else wants to support this? Since we already have an option for super, I think it makes sense. \

@platinumazure platinumazure self-assigned this Oct 24, 2019
@platinumazure
Copy link
Member

platinumazure commented Oct 24, 2019

I'll champion this. I'll try to get two more team members to support this week.

Copy link
Member

btmills left a comment

Thanks for this, @sripberger! Leaving this review as "request changes" until we decide whether to change the default or do this as a breaking change in a major version (I'm in favor of the latter).

docs/rules/no-underscore-dangle.md Show resolved Hide resolved
Copy link
Member

btmills left a comment

@sripberger explained why this would not cause new errors, so this can be merged as a semver-minor change.

@mdjermanovic mdjermanovic added accepted and removed evaluating labels Oct 28, 2019
@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 28, 2019

This is accepted now.

Copy link
Member

platinumazure left a comment

The code/docs/tests look good to me, thanks!

Only one request: If you have time, could you please ensure the commit message says allowAfterThisConstructor rather than allowAfterConstructor?

You can do this one of two ways:

  1. Amend the commit on your local branch using git commit --amend, then force push to the remote branch with git push origin issue11488 --force
  2. Push another commit with some other change to this branch, then change the PR title

Thanks!


EDIT: Thinking about it, we could also just fix that on merge. But if you have time to fix the message on your end, it will reduce the chance of a mistake on our end. 😄

@sripberger
Copy link
Contributor Author

sripberger commented Oct 29, 2019

The code/docs/tests look good to me, thanks!

Only one request: If you have time, could you please ensure the commit message says allowAfterThisConstructor rather than allowAfterConstructor?

You can do this one of two ways:

  1. Amend the commit on your local branch using git commit --amend, then force push to the remote branch with git push origin issue11488 --force
  2. Push another commit with some other change to this branch, then change the PR title

Thanks!

EDIT: Thinking about it, we could also just fix that on merge. But if you have time to fix the message on your end, it will reduce the chance of a mistake on our end.

I'll go ahead and to the force push since that's alright with you. Will be easiest.

@sripberger sripberger force-pushed the sripberger:issue11488 branch from e977b2d to 84c3a52 Oct 29, 2019
@sripberger sripberger changed the title New: allowAfterConstructor for no-underscore-dangle (fixes #11488) New: allowAfterThisConstructor for no-underscore-dangle (fixes #11488) Oct 29, 2019
Copy link
Member

platinumazure left a comment

Looks good, thanks for contributing!

@platinumazure platinumazure merged commit e17fb90 into eslint:master Oct 29, 2019
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message Commit message follows guidelines
Details
continuous-integration Build #20191029.7 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@platinumazure
Copy link
Member

platinumazure commented Oct 29, 2019

I've gone ahead and merged this. Thanks so much for the rule enhancement! I apologize for the significant delay on accepting and merging this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.