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

NewlineAtEndOfFile default should be OS agnostic #4073

Closed
jayvdb opened this issue Mar 21, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@jayvdb
Copy link

commented Mar 21, 2017

Currently the default for NewlineAtEndOfFile is system, which means that checkstyle will fail on a clean repository VCS checkout on either Windows or Unix if the EOLs where not converted to native EOLs by the VCS client, if that is even an option available in the client.

If someone very definitely wanted a certain EOL type, then surely they need a (new?) check which confirms all EOLs throughout all files are using that EOL type, not merely the EOLs at EOF as done by NewlineAtEndOfFile.

I think NewlineAtEndOfFile default of lf_cr_crlf (added #1045) more closely matches what the configuration name implies.

That will mean the ruleset https://raw.githubusercontent.com/noveogroup/android-check/master/android-check-plugin/src/main/resources/checkstyle/checkstyle-hard.xml will work as is on Windows, without depending on the git config of the users or requiring creation of .gitattributes, etc

@jayvdb

This comment has been minimized.

Copy link
Author

commented Mar 21, 2017

A scenario that will also fail is a cygwin git clone with a native Windows java.

Also note that the current default on Unix will happily accept files with crlf due to #4074 , so the current behaviour on Unix is "crlf or lf".

@Vampire

This comment has been minimized.

Copy link
Contributor

commented Mar 5, 2019

I also second this request.
I just tried to do all verifications on a checkstyle clone.
I don't like any VCS to mangle with my files, so I never let Git randomly replace line endings.
As I'm on Windows where I tried this, the NewlineAtEndOfFile check of course failed.

Currently the default is system wich means on Linux lf and crlf are accepted but on Windows only crlf is accepted.
I don't think that cr as sole line ending must be supported anymore by default, as it is legacy and should barely be used.
But lf on Windows is quite common, especially when you use Git.

So I'd suggest to change the default of this check to lf at least, or like @jayvdb suggested, use the behavior of lf_cr_crlf always and have a separate check that can check for type of line endings in all lines of all files.

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Issue checkstyle#4073: Use lf_cr_crlf as default for NewlineAtEndOfFi…
…le check

This matches more the intention of the check and also works
properly if you for example have files with Linux line ending
on Windows systems as is common for cross-platform projects.

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Issue checkstyle#4073: Use lf_cr_crlf as default for NewlineAtEndOfFi…
…le check

This matches more the intention of the check and also works
properly if you for example have files with Linux line ending
on Windows systems as is common for cross-platform projects.

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 5, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 10, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 11, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

@jayvdb and @Vampire ,
Such default is used from the beginning of This Check (16 years ago)
59aa156

change of default might break behavior in other group of users.

Why you can simply adjust your configs to use mode that you need ?

@jayvdb

This comment has been minimized.

Copy link
Author

commented Mar 17, 2019

In my case, I am not asking for myself, but because I want source analysers and test suites that are not dependent on how the files were checked out, unless the user quite explicitly wants that. Usually EOL settings on Windows are a user preference.

Other rulesets inherit the default of this rule, so they get the strange sets of behaviour of this rule without realising it. It isnt easy to find the problem - it took me a while to realise why the windows machines were having problems. Other rulesets could change the default by using the more permissive value, but then I need to raise this type of issue with each published ruleset which other users are using.

Changing NewlineAtEndOfFile to be more permissive of all types of EOL isnt likely to break anyone; more likely is the inverse -- someone expecting it to enforce system EOL would have that expectation removed by the default change.

But as explained above, that possible person really shouldnt be relying on this rule to enforce system EOL, as this rule is only about the end-of-file-EOL. A checker for the type of EOL used should apply to all of the EOLs in the file, not just the final EOL.

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 18, 2019

@Vampire

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

@romani of course we can adjust the config.
But I feel like @jayvdb, that system is not a sensible default that makes most users happy and work as expected.
Leaving the deprecated single CR out of the discussion as no recent OS uses it as default, changing to LF as default will not add violations to any project, but only make it work like expected on Windows with Linux line-endings as explained above. Using LF_CR_CRLF would then even serve the users with CR line endings no matter on which OS.
So even if this was the default from the start of this check I think a new default that is a bit more permissive is more in the spirit of this check, as it only validates that there is a line break character in the end of the file and does not check line ending types generally.
A system default that is neither LF, CR or CRLF can imho be ignored at least for the default case, as no sane person should use anything different. And if so, they can explicitly set it to system. But I broadly state that this will hit less than 0.1% of the Checkstyle userbase.

@romani

This comment has been minimized.

Copy link
Member

commented Apr 27, 2019

thanks to @Vampire and @jayvdb for comments.

Ok, I am fine with change, even it become minor breaking compatibility.

@rnveach , please review this issue and if you are agree, lets mark it approved and proceed with PRs review/merge.

@romani romani assigned rnveach and unassigned romani Apr 27, 2019

@rnveach rnveach removed their assignment Apr 27, 2019

@rnveach rnveach added the approved label Apr 27, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue May 13, 2019

@romani romani closed this in #6510 May 18, 2019

romani added a commit that referenced this issue May 18, 2019

@romani romani added this to the 8.21 milestone May 18, 2019

@romani

This comment has been minimized.

Copy link
Member

commented May 18, 2019

fix is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.