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

no-multiple-empty-lines: Adjust reported loc #12594

Merged
merged 1 commit into from Dec 20, 2019

Conversation

@Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Nov 23, 2019

The + 1 was from a time where we warned about anything with more than one empty line, but since then the rule has become configurable. We should put the warning on the first line that breaks this rule, instead of on the first empty line.

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 rule do you want to change?

no-multiple-empty-lines

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

It should produce the same number of warnings as before

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

New default behavior as it should probably be considered a bug fix

Please provide some example code that this change will affect:

console.log('foo');




console.log('foo');

What does the rule currently do for this code?

it warns on all empty lines

What will the rule do after it's changed?

it warns only on the empty lines that breaks the rule (which is the third empty line for the default configuration)

What changes did you make? (Give an overview)

I adjusted the loc of the rule report to take the rule configuration into consideration

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

nothing in particular

@jsf-clabot
Copy link

@jsf-clabot jsf-clabot commented Nov 23, 2019

CLA assistant check
All committers have signed the CLA.

@Turbo87 Turbo87 force-pushed the Turbo87:multiple-lines branch from 731672d to 656748f Nov 23, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 25, 2019

Can you add a test that would have failed before but would pass now?

@Turbo87 Turbo87 force-pushed the Turbo87:multiple-lines branch from 656748f to 1491368 Nov 25, 2019
@Turbo87
Copy link
Contributor Author

@Turbo87 Turbo87 commented Nov 25, 2019

@kaicataldo done, thanks for the review :)

The `+ 1` was from a time where we warned about anything with more than one empty line, but since then the rule has become configurable. We should put the warning on the first line that breaks this rule, instead of on the second empty line.
@Turbo87 Turbo87 force-pushed the Turbo87:multiple-lines branch from 1491368 to 1c5abc6 Nov 25, 2019
@kaicataldo kaicataldo self-assigned this Nov 25, 2019
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 25, 2019

I'll champion this!

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Nov 25, 2019

As per our process, we'll need to reach consensus by having support from three other team members. Thanks for contributing!

@Turbo87
Copy link
Contributor Author

@Turbo87 Turbo87 commented Dec 19, 2019

@kaicataldo is there anything I can do to get more eyes on this? :)

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 19, 2019

I actually think this should be treated as a bug fix. Will mark as accepted since I was able to verify this behavior (see example in demo.

@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 19, 2019

Will leave this open for another day or two to make sure there aren't any objections from the team and, if not, will merge then. Thanks for your patience!

@kaicataldo kaicataldo merged commit 272e4db into eslint:master Dec 20, 2019
18 checks passed
18 checks passed
@github-actions
Verify Files
Details
@github-actions
Test (ubuntu-latest, 13.x)
Details
@github-actions
Test (ubuntu-latest, 12.x)
Details
@github-actions
Test (ubuntu-latest, 10.x)
Details
@github-actions
Test (ubuntu-latest, 8.x)
Details
@github-actions
Test (ubuntu-latest, 8.10.0)
Details
@github-actions
Test (windows-latest, 12.x)
Details
@github-actions
Test (macOS-latest, 12.x)
Details
@github-actions
Browser Test
Details
@eslint-deprecated
commit-message Commit message follows guidelines
Details
@azure-pipelines
continuous-integration Build #20191125.12 succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
@azure-pipelines
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
@eslint-deprecated
release-monitor No patch release is pending
Details
@kaicataldo
Copy link
Member

@kaicataldo kaicataldo commented Dec 20, 2019

Thanks for contributing to ESLint!

@Turbo87
Copy link
Contributor Author

@Turbo87 Turbo87 commented Dec 20, 2019

no, no, thank you for maintaining it! :)

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jun 19, 2020
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

4 participants