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

add newline at EOF - POSIX compliance #839

Closed
wants to merge 1 commit into from

Conversation

tessus
Copy link
Collaborator

@tessus tessus commented Oct 2, 2018

POSIX [1] defines a line as follows:

3.206 Line

A sequence of zero or more non-<newline> characters plus a terminating <newline> character.

Therefore, lines not ending in a newline character aren't considered actual lines.
That's why some programs have problems processing the last line of a file if it isn't newline terminated.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206

Please note: I marked the PR as invalid so that it isn't counted towards Hacktoberfest. However, please consider merging it. ;-)

POSIX [1] defines a line as follows:

---
3.206 Line

A sequence of zero or more non- <newline> characters plus a terminating <newline> character.
---

Therefore, lines not ending in a newline character aren't considered actual lines.
That's why some programs have problems processing the last line of a file if it isn't newline terminated.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_206
@tessus tessus added the invalid label Oct 2, 2018
@laurent22
Copy link
Owner

Sorry at this point I won't apply any cosmetic changes to the code base. The way I see it is that it doesn't bring a massive improvement and then it needs to be enforced by tools, which is an additional dependency and another thing that can go wrong.

There's also the small risk that it will break existing tools in subtle way, so I prefer to avoid.

So thanks for your efforts but I won't merge this kind of pull request at this point. Perhaps I should have made this clear in contributing.md (although I guess it falls under "don't change whitespace!" ;-) )

@laurent22 laurent22 closed this Oct 4, 2018
@tessus
Copy link
Collaborator Author

tessus commented Oct 4, 2018

It's of course your code, but please let me make this clear.

This is not a cosmetic or whitespace change. In the contrary, it makes your files POSIX compliant and thus all programs/applications will recognize the last line as a line.
Currently these 407 files do not have a valid last line and there are tools out there that will break.

it needs to be enforced by tools

Not sure what you mean. You use eslint anyway, don't you? There's a flag for noeol errors.
But as soon as this is fixed, no more issues. Editors won't remove the eol.

There's also the small risk that it will break existing tools in subtle way

I think it is the other way around. Try to concatenate 2 of your files (by using cat) and you already have a mess.
wc -l does not even recognize the last line as a line and thus it is not counted. So in fact you already broke several tools. These are just 2 examples.

But as I said, it is your prerogative. I just wanted to make clear that this is not a cosmetic change.

@laurent22
Copy link
Owner

I see what you mean and I agree it can create problem if a script expects all files to be newline terminated, however for now I consider it's not a a huge problem. There's no linter at the moment (again to keep things simple).

@tessus
Copy link
Collaborator Author

tessus commented Oct 4, 2018

Sure, let me know when you change your mind. ;-) I wrote a script for finding files and adding a newline at EOF if not present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants