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: no-multiple-empty-lines fixer (fixes #6225) #6226

Merged
merged 1 commit into from Jun 13, 2016

Conversation

RRMoelker
Copy link
Contributor

Multiple empty lines rule fixer. Removes empty lines at beginning, middle or end of file. An array of lines is created along with an array of range starts. This is required to define the deletion ranges from line indices.

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @vitorbal, @gcochard and @jaredsohn to be potential reviewers

@alberto
Copy link
Member

alberto commented May 19, 2016

You should add tests for windows line endings: \r\n

@nzakas
Copy link
Member

nzakas commented May 21, 2016

Agreed. @RRMoelker can you add some test with Windows line endings?

@RRMoelker
Copy link
Contributor Author

I will try to add support for the Windows line endings this weekend.

@platinumazure platinumazure added the do not merge This pull request should not be merged yet label May 27, 2016
@platinumazure
Copy link
Member

Adding "do not merge" until Windows line ending tests come in.

@eslintbot
Copy link

LGTM

@RRMoelker
Copy link
Contributor Author

Windows line endings were not being fixed correctly. Nice catch. The problem is that the SourceCode.lines variable contains lines without the newline character(s). This workaround works. Though at it's core translating between source code position and ranges is just not complete enough to make great fixers at the moment.

@eslintbot
Copy link

LGTM

@RRMoelker
Copy link
Contributor Author

@nzakas @platinumazure Windows line ending support and tests added.

@nzakas nzakas removed the do not merge This pull request should not be merged yet label Jun 6, 2016
@nzakas
Copy link
Member

nzakas commented Jun 6, 2016

Lgtm. Thanks @RRMoelker !

@nzakas
Copy link
Member

nzakas commented Jun 6, 2016

Oops, one last thing: can you update the documentation? There are two places to update:

  1. https://github.com/eslint/eslint/blob/master/docs/rules/README.md put "(fixable)" after the rule description
  2. https://github.com/eslint/eslint/blob/master/docs/rules/no-multiple-empty-lines.md insert a notice about it being fixable at the top (you can copy from https://github.com/eslint/eslint/blob/master/docs/rules/space-in-parens.md)

@ilyavolodin
Copy link
Member

@RRMoelker quick ping to update documentation.

@RRMoelker
Copy link
Contributor Author

@ilyavolodin I will update the documentation this weekend.

@ilyavolodin
Copy link
Member

Awesome! Thanks!

@eslintbot
Copy link

LGTM

@ilyavolodin
Copy link
Member

LGTM. Thanks @RRMoelker

@nzakas nzakas merged commit 5122f73 into eslint:master Jun 13, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@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 Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants