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

Prohibiting two consecutive newlines at EOF does not work as expected #12742

Closed
ghost opened this issue Jan 4, 2020 · 12 comments
Closed

Prohibiting two consecutive newlines at EOF does not work as expected #12742

ghost opened this issue Jan 4, 2020 · 12 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation good first issue Good for people who haven't worked on ESLint before help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules

Comments

@ghost
Copy link

ghost commented Jan 4, 2020

Tell us about your environment

  • ESLint Version: ^6.8.0
  • Node Version: 10.0.0
  • npm Version: 5.6.0

What parser (default, Babel-ESLint, etc.) are you using? default (?)

Please show your full configuration:

Configuration
{
  "env": {
    "es6": true,
    "node": true
  },
  "extends": [
    "airbnb-base"
  ],
  "globals": {
    "Atomics": "readonly",
    "SharedArrayBuffer": "readonly"
  },
  "parserOptions": {
    "ecmaVersion": 2018,
    "sourceType": "module"
  },
  "rules": {
    "no-multiple-empty-lines": ["error", { "max": 1 }]
  }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Source code:

// Source code of any .js file of the project

Command:

npx eslint .

What did you expect to happen? What actually happened?

I want to ensure there is exactly one newline at the end of each file. Since I am extending the Airbnb style guide, eol-last is already enabled, which afaik requires at least one newline at the end of the file. So all that's left for me to specify is that more than one newlines at the end of the file are disallowed. I thought I might be able to accomplish this like so:

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

Now, there should be both

  • the requirement of at least one newline at the end of the file, due to eol-last being enabled in the Airbnb style guide, as well as
  • the requirement of no more than one newline at a time (applying to the whole file, not just the end of the file), due to manually setting no-multiple-empty-lines to { "max": 1 }.

However, this still configuration seems to still allow two consecutive newlines at the end of the file.

In the Gitter chat room, it was recommended to set maxEOF to 1. This, however, does not change the fact that two newlines at the end of the file are being accepted. The fact that this does not fix the issue is to be expected, as max is already set to 1, so setting maxEOF to 1 also should not have an effect.

In the Gitter chat room it was concluded to file a bug report.

Are you willing to submit a pull request to fix this bug?

Maybe. It would be an honor but I'm not sure I can. I'll take a look.

@ghost ghost added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jan 4, 2020
@kaicataldo kaicataldo assigned aladdin-add and unassigned aladdin-add Jan 4, 2020
@kaicataldo
Copy link
Member

kaicataldo commented Jan 4, 2020

I’m unable to reproduce this. I know you mentioned you did try the maxEOF option in the Gitter chat, but it looks like that is working correctly (see demo here).

I’d have to look at the Git history, but I’m assuming the maxEOF option exists so that this rule can be configured to not conflict with eol-last.

@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Jan 4, 2020
@ghost
Copy link
Author

ghost commented Jan 4, 2020

Like I described above: Setting maxEOF to 1 should not make a difference, since max is already set to 1. In the docs it says:

maxEOF can be used to set a different number for the end of file. The last blank lines will then be treated differently. If omitted, the max option is applied at the end of the file.

And it really doesn't make a difference, two consecutive lines at EOF are being permitted -- no matter whether maxEOF is explicitly set or not.

@mdjermanovic
Copy link
Member

eol-last doesn't require an empty line at the end of the file. It requires that the last character in the source code is \n. There is no empty line at the end of the file after that last \n, although it looks like that in editors. You'd probably want to set maxEOF to 0.

@ghost
Copy link
Author

ghost commented Jan 5, 2020

This certainly solved my issue. Thus, closing this issue is fine by me. However, you guys might want to leave it open, as this behavior is really non-intuitive. Without the explanation that @mdjermanovic just gave, it's really hard to grasp why maxEOF has to be set to 0 to enforce exactly one line at the EOF. Also, the documentation gives no clue about this.

@mdjermanovic
Copy link
Member

maxEOF has to be set to 0 to enforce exactly one line at the EOF. Also, the documentation gives no clue about this.

I believe it's technically zero empty lines at the EOF, but it looks like one empty line in most editors.

I agree this should be explained in the documentation. Also, I think there should be an example with maxEOF: 0 because it's a common configuration for this rule. PR is welcome!

Just, I'm not sure how to avoid this confusion in maxEOF examples because the presentation depends on the editor/viewer.

For instance, if you open to view a source file on GitHub and see a code that looks exactly like the actual incorrect example in the docs (GitHub shows 8 lines, last 2 are empty), it's indeed an incorrect code by this rule. If you open the same file in VSCode, there will be one extra line (VSCode shows 9 lines, last 3 are empty).

However, if you open another source file in VSCode, and see a code that looks exactly like the actual incorrect example in the docs (VSCode shows 8 lines, last 2 are empty), it's actually a correct code by this rule.

image

It's same in ESLint Online Demo

So, these examples can be indeed confusing.

Also, it looks that some other rules like max-lines inconsistently treat this issue, because it seems that SourceCode#lines does contain an extra line after the last linebreak.

@kaicataldo
Copy link
Member

Right, an empty line is actually \n\n. Would be great to clarify this in the documentation!

@mdjermanovic
Copy link
Member

There is an option in VSCode to modify this behavior: "editor.renderFinalNewline": false

And it seems to be true by default on Windows only, false otherwise.

Also, the question does a newline/linebreak/eol sequence ends the current line or creates a new line (i.e. is there an empty line after the last \n) may be just a matter of convention in a particular environment, so it might be best to be neutral in the documentation - to just explain that editors/viewers/tools render and treat this differently and say that the rule doesn't count this as an empty line.

I was thinking to provide both views on the same examples in the docs - one representing a screenshot of a viewer that doesn't render an extra empty line, the other representing a viewer that does render an extra empty line. (not real screenshots, just code blocks with extra lines)

Does this seem like a good idea?

There is also the following potential problem: code examples in the documentation have a different appearance on eslint.org and GitHub.

This looks like 2 empty lines at the end:

image

This looks like 0 empty lines at the end:

image

There are three \n\n\n on the site after var bar = 3;, while the GitHub viewer renders only one \n.

This usually doesn't matter but in this particular case users might already get the wrong impression on how the rule works at the EOF depending on where they're reading the documentation.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation and removed bug ESLint is working incorrectly evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jan 16, 2020
@mdjermanovic
Copy link
Member

It seems we all agree that the rule is working as intended and that it would be great to improve the documentation, so I marked this as an accepted "documentation" issue.

@kaicataldo
Copy link
Member

kaicataldo commented Jan 16, 2020

I was thinking to provide both views on the same examples in the docs - one representing a screenshot of a viewer that doesn't render an extra empty line, the other representing a viewer that does render an extra empty line. (not real screenshots, just code blocks with extra lines)

Does this seem like a good idea?

Sounds good to me! I think it would helpful to show this with line numbers anyway.

@mdjermanovic
Copy link
Member

Sounds good to me! I think it would helpful to show this with line numbers anyway.

That's a great idea!

Is there already a way to show line numbers in code blocks (at least on eslint.org)?

@kaicataldo
Copy link
Member

We don't have anything set up as far as I know, but it might be worth looking at the options for the Markdown -> HTML plugins we're using in Eleventy to see if there's a built-in way to do it. If not, we can implement it. That being said, I don't think that needs to be a blocker for the documentation update!

@kaicataldo kaicataldo added good first issue Good for people who haven't worked on ESLint before help wanted The team would welcome a contribution from the community for this issue labels Feb 7, 2020
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Apr 5, 2020
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that accepted issues failing to be implemented after 90 days tend to
never be implemented, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@mdjermanovic mdjermanovic reopened this Apr 5, 2020
@mdjermanovic mdjermanovic self-assigned this Apr 5, 2020
@mdjermanovic mdjermanovic removed the auto closed The bot closed this issue label Apr 6, 2020
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue May 30, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue May 30, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue May 30, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue Jun 4, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue Jun 4, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
arthurdias-trad added a commit to arthurdias-trad/eslint that referenced this issue Jun 5, 2020
Added an explanation on how and why maxEOF should be set to 0 to work with the eol-last rule.
@nzakas nzakas closed this as completed in 1ee3c42 Jun 11, 2020
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jun 2, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jun 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation good first issue Good for people who haven't worked on ESLint before help wanted The team would welcome a contribution from the community for this issue rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

3 participants