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

rule new-lines: checks only the first newline character in a file #475

Open
Cube707 opened this issue Jun 21, 2022 · 4 comments · May be fixed by #693
Open

rule new-lines: checks only the first newline character in a file #475

Cube707 opened this issue Jun 21, 2022 · 4 comments · May be fixed by #693

Comments

@Cube707
Copy link
Contributor

Cube707 commented Jun 21, 2022

Currently, the check() function of rules/new_lines.py is only called once per file, checking only the first new-line-character of a file. (see also this comment)

To also be able to detect mixed newlines (and report the wrong ones) all lines of a file should be checked.

Example

Consider this yaml-file (with the newline characters explicitly shown)

---\n
test\r\n
\n

If it is checked for UNIX-style newlines, it should report an error on line 2 and if it is checked for DOS-style newlines it should report two errors on lines 1 and 3.

Currently, this file would be valid if checked with type: unix and invalid with type: dos (with only one error on line 1).

Discussion

It should be discussed if all wrong newline characters should be reported.

In my opinion, this would clutter the error output too much and the amount of errors resulting from this should be caped. Either only one new-lines error per file or only a small amount (maybe max of 5?). Then the error message should say something like "more new-line errors possible, but not showing..."


side effects

PR #474 introduced a new type and tests for it. Some test where supposed to test this type of scenario and therefore are currently broken. They can be reactivated once this issue is fixed. See:

# FIXME: the following tests currently don't work
# because only the first line is checked for line-endings
# see: issue #475
# ---
# self.check('---\ntext\r\nfoo\n', conf, problem=(2, 4))
# self.check('---\ntext\r\n', conf, problem=(2, 4))

# FIXME: the following tests currently don't work
# because only the first line is checked for line-endings
# see: issue #475
# ---
# self.check('---\r\ntext\nfoo\r\n', conf, problem=(2, 4))
# self.check('---\r\ntext\n', conf, problem=(2, 4))

@adrienverge
Copy link
Owner

Hello,

Thanks for spinning off that issue from #474 (comment).

I agree at most one error per file should be output, to avoid flooding the output, to keep a similar behavior as today, and to be consistent with other rules like syntax.

@dbk-rabel
Copy link

Hi there.

Is this still planned to be implemented? Because it would be incredibly helpful for us.

In general you have files of the same line-ending and want to get notified when someone changes a few lines to a wrong line-ending. So in my opinion it would even be right to have an output for each line with wrong line-ending. But of course one warning per file would be enough, because you would probably fix it with dos2unix anyways.

The important part is that it's not enough to check only the first line-ending per file.

Yours
David

@adrienverge
Copy link
Owner

Hello David, would you like to implement it?

@dbk-rabel
Copy link

@adrienverge I tried at least :) #693 looks good now. Could you have a look at it?

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 a pull request may close this issue.

3 participants