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

wrong count for 'no-multiple-empty-lines' on last line #4228

Closed
le0nik opened this Issue Oct 22, 2015 · 15 comments

Comments

Projects
None yet
5 participants
@le0nik

le0nik commented Oct 22, 2015

Eslint v1.7.3
In .eslintrc

Happens on the last line.

 "no-multiple-empty-lines": [2, {"max": 1}],

throws error with this code:

1 console.log(a);
2 <---- (error, although only 1 empty line)
2:1  error  Multiple blank lines not allowed  no-multiple-empty-lines

this rule

 "no-multiple-empty-lines": [2, {"max": 2}],

doesn't throw for this code

1 console.log(a);
2 

but does for this one

1 console.log(a);
2 
3 <----(error, although only two empty lines)
3:1  error  Multiple blank lines not allowed  no-multiple-empty-lines
@eslintbot

This comment has been minimized.

Show comment
Hide comment
@eslintbot

eslintbot Oct 22, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

eslintbot commented Oct 22, 2015

Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@eslintbot eslintbot added the triage label Oct 22, 2015

@le0nik le0nik changed the title from wrong count for 'no-multiple-empty-lines' to wrong count for 'no-multiple-empty-lines' on last line Oct 22, 2015

@gyandeeps

This comment has been minimized.

Show comment
Hide comment
@gyandeeps

gyandeeps Oct 22, 2015

Member

Cannot reproduce it on online demo. have you tried it here?

Member

gyandeeps commented Oct 22, 2015

Cannot reproduce it on online demo. have you tried it here?

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas
Member

nzakas commented Oct 22, 2015

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

I'll look at this

Member

alberto commented Oct 23, 2015

I'll look at this

@le0nik

This comment has been minimized.

Show comment
Hide comment
@le0nik

le0nik Oct 23, 2015

@gyandeeps can't reproduce it in online demo. One can't choose the number of max lines there, so i don't know whether it works correctly or not.

le0nik commented Oct 23, 2015

@gyandeeps can't reproduce it in online demo. One can't choose the number of max lines there, so i don't know whether it works correctly or not.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

Ok, it turns out I didn't know what is a linebreak. :)

A linebreak doesn't really add a new line, just terminates the current line.

This code has just one line of code, linebreak terminated:

var a = 5;\n

As shown by wc output:

wc -l linebreak.js
       1 linebreak.js

And the file output displaying non-printing characters

cat -e linebreak.js
var a = 5;$

However context.getSourceLines() returns 2 lines:

console.log(context.getSourceLines())
[ 'var a = 5;', '' ]

And editors display that file differently:

Some, like Atom and Sublime Text 3 pretend there is an empty line for you to keep writing, when there is a linebreak terminating the line.
atom

I believe the same thing is happening in the online editor

Others, like vim don't
vim

Given all that, I'm not sure what should be done. Now I'm thinking this fix was wrong, although the reporting is confusing in editors such as atom.

Thoughts @eslint/eslint-team ?

Member

alberto commented Oct 23, 2015

Ok, it turns out I didn't know what is a linebreak. :)

A linebreak doesn't really add a new line, just terminates the current line.

This code has just one line of code, linebreak terminated:

var a = 5;\n

As shown by wc output:

wc -l linebreak.js
       1 linebreak.js

And the file output displaying non-printing characters

cat -e linebreak.js
var a = 5;$

However context.getSourceLines() returns 2 lines:

console.log(context.getSourceLines())
[ 'var a = 5;', '' ]

And editors display that file differently:

Some, like Atom and Sublime Text 3 pretend there is an empty line for you to keep writing, when there is a linebreak terminating the line.
atom

I believe the same thing is happening in the online editor

Others, like vim don't
vim

Given all that, I'm not sure what should be done. Now I'm thinking this fix was wrong, although the reporting is confusing in editors such as atom.

Thoughts @eslint/eslint-team ?

@alberto alberto added bug rule labels Oct 23, 2015

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 23, 2015

Member

Can you explain why you think that fix was wrong?

Member

nzakas commented Oct 23, 2015

Can you explain why you think that fix was wrong?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

@nzakas It depends on wether you consider the example I showed has 0 or 1 empty lines at the end.

Member

alberto commented Oct 23, 2015

@nzakas It depends on wether you consider the example I showed has 0 or 1 empty lines at the end.

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

@gyandeeps what's your editor?

Can you do a cat -e <file>?

Member

alberto commented Oct 23, 2015

@gyandeeps what's your editor?

Can you do a cat -e <file>?

@le0nik

This comment has been minimized.

Show comment
Hide comment
@le0nik

le0nik Oct 23, 2015

@alberto i suppose, you wanted to ask me.

My editor is macvim with syntastic.

Here's the result of running cat -e <file> command:

console.log('hello');$
$

le0nik commented Oct 23, 2015

@alberto i suppose, you wanted to ask me.

My editor is macvim with syntastic.

Here's the result of running cat -e <file> command:

console.log('hello');$
$
@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

Yes @le0nik, thanks

Member

alberto commented Oct 23, 2015

Yes @le0nik, thanks

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 23, 2015

Member

@alberto can you break it down for me? In what scenario was the fix wrong? Why was it wrong?

Member

nzakas commented Oct 23, 2015

@alberto can you break it down for me? In what scenario was the fix wrong? Why was it wrong?

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

If you consider the linebreak to be a line terminator but not creating a new line itself, then I think the original code was doing the right thing removing the last line if it was empty.

As you can see from the screenshots I posted, when you open the same file with vim it appears to have one line. With atom it looks like it has two. I don't think there is a way for them to look correct from a user perspective, but technically the original behaviour seems the correct one so I would favor that.

Member

alberto commented Oct 23, 2015

If you consider the linebreak to be a line terminator but not creating a new line itself, then I think the original code was doing the right thing removing the last line if it was empty.

As you can see from the screenshots I posted, when you open the same file with vim it appears to have one line. With atom it looks like it has two. I don't think there is a way for them to look correct from a user perspective, but technically the original behaviour seems the correct one so I would favor that.

@nzakas

This comment has been minimized.

Show comment
Hide comment
@nzakas

nzakas Oct 23, 2015

Member

Thanks for explaining, I agree. Do you want to make that change?

Member

nzakas commented Oct 23, 2015

Thanks for explaining, I agree. Do you want to make that change?

@nzakas nzakas added accepted and removed triage labels Oct 23, 2015

@alberto

This comment has been minimized.

Show comment
Hide comment
@alberto

alberto Oct 23, 2015

Member

yes, let me fix that

Member

alberto commented Oct 23, 2015

yes, let me fix that

@alberto alberto closed this in 13aae50 Oct 24, 2015

nzakas added a commit that referenced this issue Oct 24, 2015

Merge pull request #4244 from eslint/issue4228
Fix: wrong count for 'no-multiple-empty-lines' on last line (fixes #4228)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.