Check if multiple newlines are at EOF #350
Conversation
ff8d233 to
d104b72
Compare
With insert_final_newline enabled, only a single final newline should be at the end of file. Add a check to fail, if there are multiple newlines. This new validation logic doesn't fail with wrong line endings anymore, there's already another validator for that. Fixes editorconfig-checker#341
d104b72 to
0eb9019
Compare
There was a problem hiding this comment.
Thank you for submitting this PR! @holesch
This mostly looks good, left a few comments, don't worry too much about CI, we have a problem on main, which make CI fails, we should resolve it ASAP (in a separated PR).
This new validation logic doesn't fail with wrong line endings anymore, there's already another validator for that.
Could you elaborate, on that please? Where is the other validator for this use case?
I would rather this PR focus only on the "multiple newlines" handling, and not editing current error messages or tests, this PR should only add new tests, but not edit existing ones, as the purpose of this PR is to add new errors when there are multiple newlines EOF.
That way, it will be easier to review and understand, why it is implemented the way it is.
Other changes, like improving error messages, or changing behavior in existing tests, should be done in separated PRs.
| {"x", "true", "lf", errors.New("Wrong line endings or no final newline")}, | ||
| {"x", "true", "cr", errors.New("Wrong line endings or no final newline")}, | ||
| {"x", "true", "crlf", errors.New("Wrong line endings or no final newline")}, | ||
| {"x", "true", "lf", errors.New("Final newline expected")}, | ||
| {"x", "true", "cr", errors.New("Final newline expected")}, | ||
| {"x", "true", "crlf", errors.New("Final newline expected")}, |
There was a problem hiding this comment.
Good idea to be more explicit in error messages, so it is easier to fix. 👍
(but as said in my review comment, it should be done in a separated PR, as it introduces false error messages for other things, see my other comment)
| {"x\n", "true", "cr", errors.New("Wrong line endings or no final newline")}, | ||
| {"x\n", "true", "crlf", errors.New("Wrong line endings or no final newline")}, | ||
| {"x\n", "true", "cr", errors.New("Final newline expected")}, | ||
| {"x\n", "true", "crlf", errors.New("Final newline expected")}, | ||
|
|
||
| {"x\r", "true", "lf", errors.New("Wrong line endings or no final newline")}, | ||
| {"x\r", "true", "crlf", errors.New("Wrong line endings or no final newline")}, | ||
| {"x\r", "true", "lf", errors.New("Final newline expected")}, | ||
| {"x\r", "true", "crlf", errors.New("Final newline expected")}, |
There was a problem hiding this comment.
The error messages here, if I understand correctly, should be "Wrong line endings".
For example line 26, with "x\n" with cr (\r) mode, it is because the final line ending should be \r and it is instead \n, but there is a final newline, it is just not the right one?
There was a problem hiding this comment.
That's what I meant in the commit message: There's validators.LineEnding, that catches this error already:
$ printf 'x\r' > test
$ ./bin/ec -format gcc test
test:
Final newline expected
Not all lines have the correct end of line character
2 errors foundI found it more logical to not test it twice, but expect \r\n as final newline and \r\n is not here.
As written in the other comment, it's
I found it easier to add this feature after removing the wrong line endings detection. What would you say if I split this in two commits? Do you agree with the line ending change or should I remove it completely? |
It's fine like that, thanks for explaining, I didn't know, there was Looks good to me! 😄 |
|
@holesch I did not check the PR but you should rebase so that CI passes. :) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #350 +/- ##
==========================================
+ Coverage 84.89% 85.50% +0.61%
==========================================
Files 8 8
Lines 695 545 -150
==========================================
- Hits 590 466 -124
+ Misses 81 55 -26
Partials 24 24 ☔ View full report in Codecov by Sentry. |
|
Presumably this is "just a fix" and aligning with editorconfig's specs, but in fact this is a breaking change for editorconfig-checker happening from 3.0.1 to 3.0.2 based on a clarification comment from March 1st 2023. Please consider reverting this and queue this for the next major release. |
|
Agreed that this is a breaking change and should've been a major release. |
The premise of this change seems flawed to me. From https://editorconfig.org/:
From these discussions in the editorconfig repo: Nothing suggests that |
|
@andreineculau @cfbao Determining which changes are breaking, is a difficult task and includes subjectivity. Because "Every change breaks someone's workflow", https://xkcd.com/1172/ Another famous linting tool, for example in the JavaScript ecosystem, ESLint, considers bug fixes that results in reporting more linting errors, is not considered breaking but a minor version, while they don't consider it a "simple" patch version, it's not considered major. (https://github.com/eslint/eslint?tab=readme-ov-file#semantic-versioning-policy) Now we might need to do the same as ESLint, add documentation about how we handle semantic versioning for editorconfig-checker. I'm not saying, we should not consider that kind of changes breaking, but that's another issue in itself that can probably be discussed separately.
Is that worth it, to release v3.0.3, which revert these changes, and directly after, release v4.0.0? Don't think so, as if these changes are considered "right"/correct, it's probably easier for users to have editorconfig-checker updated to the latest version, and without false positives. Coming back to the real "issue"/bug fix, another famous implementation of editorconfig, e.g: https://github.com/editorconfig/editorconfig-vscode, removes extra end of lines, when IMO that's the expected behavior, I might miss something, but what are the use cases to include multiple end of lines at the end of a file, one is enough? |
|
I can appreciate the nuance and difficulty in versioning. Have experienced multiple times myself.
This isn't true in my experience. I do want a final new line in all files. I also do not want EC checker to remove multiple new lines. So One example where having two empty lines at the end helps is in a markdown template like this: People using the template should fill it out like not Not having two line endings can easily lead to the second less desirable outcome. |
|
+1 for the subjectivity of semver compat. The highlight in "Please consider" is on please 🤝
Also in the same camp. "unset" is no go Don't think it's useful to talk concrete examples but fwiw our case is sqlfmt which "fixes" some files by introducing an empty trailing line. It's worth the comparison with the vscode extension: the difference is in plugin vs standalone. In vscode the editorconfig extension will format the file (so maybe it removes the trailing empty lines) but the sqlfmt extension has precedence and wins (adds the empty line). So while it's great to try to align, aligning for the sake of aligning would only work if the premise is the same. It isn't. PS: I haven't checked vscode behavior but I incline to agree with @cfbao as the extension's source code doesn't mention any line removal https://github.com/editorconfig/editorconfig-vscode/blob/main/src/transformations/InsertFinalNewline.ts . Maybe we're seeing a side-effect of the built-in file.trimTrailingWhitespace setting set to true?! PPS: I also read the other editorconfig threads and I agree with @cfbao - the premise seems false. insert_new_line should make sure there is a (i.e. at least one) new line. Otherwise we agree we increase cognitive load by needing to read the fine print as "insert" is not anymore the English "insert" |
|
Hi, sorry for breaking things, didn't think this would be so controversial, but the relevant xkcd was already linked :). So IMO the vast majority of files would benefit from this fix/change. There are always edge-cases, where this is unwanted, but couldn't you exclude only those special files with a narrow match? For example: [*]
insert_final_newline = true
[docs/templates/*.md]
insert_final_newline = unsetRegarding major bump or not, I just saw the "bug" label in #341 and assumed "fix" would be appropriate. |
IMO the vast majority of files gain nothing from this change. I'm seeing multiple unrelated repos across multiple teams in our org encountering this problem right now.
No. That'd mean we're allowing files without an trailing newline in the matched file pattern, which is false. |
My bad, you are right, I always thought it was the Basically nearly all the files, I'm used to edit inside VSCode, are formatted on save, and remove the extra lines. While searching on the editorconfig repo, I found this issue: editorconfig/editorconfig#269 While, to me, it still doesn't make sense to have files with multiple final new lines (and as demonstrated above, many famous formatters/tools already remove those extra lines), and would like to keep the current behavior as of v3.0.2, the best option, would be to wait until it is officially an option in editorconfig to remove extra lines. Meanwhile, I think, we can revert this PR, as editorconfig-checker should follow the editorconfig spec, not creating "new rules". |
This reverts commit 64b76e7.
With insert_final_newline enabled, only a single final newline should be at the end of file. Add a check to fail, if there are multiple newlines.
This new validation logic doesn't fail with wrong line endings anymore, there's already another validator for that.
Fixes #341