Checkstyle "NewlineAtEndOfFile" rule issue

[agura]

Igniters,

recently I run build with checkstyle profile on Windows machine and
got 8 issues related to the "NewlineAtEndOfFile" rule while there are
no problems on my Linux machine.

I investigated the problem and suggest explicitly configure this rule
by LF separator. See [1] for more details.

I want merge this change if there are no any objections from community members.

[1] https://issues.apache.org/jira/browse/IGNITE-12472

[Maxim Muzafarov]

Hello, Andrey

Is it better to upgrade the checkstyle plugin version?
It seems the issue has been fixed since 8.21 version (currently we have 8.19)

[1] https://checkstyle.org/releasenotes.html#Release_8.21

On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:

>
> Igniters,
>
> recently I run build with checkstyle profile on Windows machine and
> got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> no problems on my Linux machine.
>
> I investigated the problem and suggest explicitly configure this rule
> by LF separator. See [1] for more details.
>
> I want merge this change if there are no any objections from community members.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-12472

[Pavel Tupitsyn]

Igniters,

Does this rule bring any value whatsoever for us?
Let's just disable it.

On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]> wrote:

> Hello, Andrey
>
> Is it better to upgrade the checkstyle plugin version?
> It seems the issue has been fixed since 8.21 version (currently we have
> 8.19)
>
> [1] https://checkstyle.org/releasenotes.html#Release_8.21
>
> On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> >
> > Igniters,
> >
> > recently I run build with checkstyle profile on Windows machine and
> > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > no problems on my Linux machine.
> >
> > I investigated the problem and suggest explicitly configure this rule
> > by LF separator. See [1] for more details.
> >
> > I want merge this change if there are no any objections from community
> members.
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-12472
>

[Maxim Muzafarov]

Pavel,

It's configured according to accepted Coding Guidelines [1].

[1] https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines

On Thu, 19 Dec 2019 at 18:59, Pavel Tupitsyn <[hidden email]> wrote:

>
> Igniters,
>
> Does this rule bring any value whatsoever for us?
> Let's just disable it.
>
> On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]> wrote:
>
> > Hello, Andrey
> >
> > Is it better to upgrade the checkstyle plugin version?
> > It seems the issue has been fixed since 8.21 version (currently we have
> > 8.19)
> >
> > [1] https://checkstyle.org/releasenotes.html#Release_8.21
> >
> > On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> > >
> > > Igniters,
> > >
> > > recently I run build with checkstyle profile on Windows machine and
> > > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > > no problems on my Linux machine.
> > >
> > > I investigated the problem and suggest explicitly configure this rule
> > > by LF separator. See [1] for more details.
> > >
> > > I want merge this change if there are no any objections from community
> > members.
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-12472
> >

[Pavel Tupitsyn]

Maxim,

The guidelines are not set in stone.
If we decide that some guideline does not bring any value and only wastes
our time (like this one),
we can (and should) remove it.

On Thu, Dec 19, 2019 at 7:13 PM Maxim Muzafarov <[hidden email]> wrote:

> Pavel,
>
> It's configured according to accepted Coding Guidelines [1].
>
> [1]
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines
>
> On Thu, 19 Dec 2019 at 18:59, Pavel Tupitsyn <[hidden email]> wrote:
> >
> > Igniters,
> >
> > Does this rule bring any value whatsoever for us?
> > Let's just disable it.
> >
> > On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]>
> wrote:
> >
> > > Hello, Andrey
> > >
> > > Is it better to upgrade the checkstyle plugin version?
> > > It seems the issue has been fixed since 8.21 version (currently we have
> > > 8.19)
> > >
> > > [1] https://checkstyle.org/releasenotes.html#Release_8.21
> > >
> > > On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> > > >
> > > > Igniters,
> > > >
> > > > recently I run build with checkstyle profile on Windows machine and
> > > > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > > > no problems on my Linux machine.
> > > >
> > > > I investigated the problem and suggest explicitly configure this rule
> > > > by LF separator. See [1] for more details.
> > > >
> > > > I want merge this change if there are no any objections from
> community
> > > members.
> > > >
> > > > [1] https://issues.apache.org/jira/browse/IGNITE-12472
> > >
>

[agura]

Agree with Pavel. But it could broke something in our pipeline.

Maxim, if issue was fixed so it dependency update is better way. But
it seems that this issue in Open status.

On Thu, Dec 19, 2019 at 7:22 PM Pavel Tupitsyn <[hidden email]> wrote:

>
> Maxim,
>
> The guidelines are not set in stone.
> If we decide that some guideline does not bring any value and only wastes
> our time (like this one),
> we can (and should) remove it.
>
> On Thu, Dec 19, 2019 at 7:13 PM Maxim Muzafarov <[hidden email]> wrote:
>
> > Pavel,
> >
> > It's configured according to accepted Coding Guidelines [1].
> >
> > [1]
> > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines
> >
> > On Thu, 19 Dec 2019 at 18:59, Pavel Tupitsyn <[hidden email]> wrote:
> > >
> > > Igniters,
> > >
> > > Does this rule bring any value whatsoever for us?
> > > Let's just disable it.
> > >
> > > On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]>
> > wrote:
> > >
> > > > Hello, Andrey
> > > >
> > > > Is it better to upgrade the checkstyle plugin version?
> > > > It seems the issue has been fixed since 8.21 version (currently we have
> > > > 8.19)
> > > >
> > > > [1] https://checkstyle.org/releasenotes.html#Release_8.21
> > > >
> > > > On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> > > > >
> > > > > Igniters,
> > > > >
> > > > > recently I run build with checkstyle profile on Windows machine and
> > > > > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > > > > no problems on my Linux machine.
> > > > >
> > > > > I investigated the problem and suggest explicitly configure this rule
> > > > > by LF separator. See [1] for more details.
> > > > >
> > > > > I want merge this change if there are no any objections from
> > community
> > > > members.
> > > > >
> > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12472
> > > >
> >

[Maxim Muzafarov]

Andrey,


I don't know why the issue [1] has an open status, but according to
release notes [2] and checkstyle-PR [3] the default behaviour for the
check `NewlineAtEndOfFile` has changed to  `lf_cr_crlf` and works.

Actually, you may probably find a long interesting discussion in this
PR [3] about which is better to check `lf` as line endings (same as
you suggested earlier) or `lf_cr_crlf` and folks have concluded to
change the rule default behaviour to `lf_cr_crlf`. So, I think it's
better for us to use their defaults.


I've prepared PR [4] with my proposal and tested it on TC and locally
(on Windows). Most of the changes related to EmptyLineSeparator rule
which has been fixed here (EmptyLineSeparator check does not validate
newlines before comments. [6]) and included to 8.20 release version.

Please, consider applying PR [4].
Also, let's add a recommendation to configure `autocrlf` option for git.

[1] https://issues.apache.org/jira/browse/MCHECKSTYLE-376
[2] https://checkstyle.org/releasenotes.html#Release_8.21
[3] https://github.com/checkstyle/checkstyle/issues/4073
[4] https://github.com/apache/ignite/pull/7174
[5] https://issues.apache.org/jira/browse/IGNITE-12472
[6] https://github.com/checkstyle/checkstyle/issues/5981
[7] https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration

On Thu, 19 Dec 2019 at 20:55, Andrey Gura <[hidden email]> wrote:

>
> Agree with Pavel. But it could broke something in our pipeline.
>
> Maxim, if issue was fixed so it dependency update is better way. But
> it seems that this issue in Open status.
>
> On Thu, Dec 19, 2019 at 7:22 PM Pavel Tupitsyn <[hidden email]> wrote:
> >
> > Maxim,
> >
> > The guidelines are not set in stone.
> > If we decide that some guideline does not bring any value and only wastes
> > our time (like this one),
> > we can (and should) remove it.
> >
> > On Thu, Dec 19, 2019 at 7:13 PM Maxim Muzafarov <[hidden email]> wrote:
> >
> > > Pavel,
> > >
> > > It's configured according to accepted Coding Guidelines [1].
> > >
> > > [1]
> > > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines
> > >
> > > On Thu, 19 Dec 2019 at 18:59, Pavel Tupitsyn <[hidden email]> wrote:
> > > >
> > > > Igniters,
> > > >
> > > > Does this rule bring any value whatsoever for us?
> > > > Let's just disable it.
> > > >
> > > > On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]>
> > > wrote:
> > > >
> > > > > Hello, Andrey
> > > > >
> > > > > Is it better to upgrade the checkstyle plugin version?
> > > > > It seems the issue has been fixed since 8.21 version (currently we have
> > > > > 8.19)
> > > > >
> > > > > [1] https://checkstyle.org/releasenotes.html#Release_8.21
> > > > >
> > > > > On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> > > > > >
> > > > > > Igniters,
> > > > > >
> > > > > > recently I run build with checkstyle profile on Windows machine and
> > > > > > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > > > > > no problems on my Linux machine.
> > > > > >
> > > > > > I investigated the problem and suggest explicitly configure this rule
> > > > > > by LF separator. See [1] for more details.
> > > > > >
> > > > > > I want merge this change if there are no any objections from
> > > community
> > > > > members.
> > > > > >
> > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12472
> > > > >
> > >

[Ivan Pavlukhin]

Can we use "autocrlf" safely? AFAIR there were problems (in other
project) with some Windows specific stuff if CRLF is replaced to LF
automatically.

пт, 20 дек. 2019 г. в 17:31, Maxim Muzafarov <[hidden email]>:

>
> Andrey,
>
>
> I don't know why the issue [1] has an open status, but according to
> release notes [2] and checkstyle-PR [3] the default behaviour for the
> check `NewlineAtEndOfFile` has changed to  `lf_cr_crlf` and works.
>
> Actually, you may probably find a long interesting discussion in this
> PR [3] about which is better to check `lf` as line endings (same as
> you suggested earlier) or `lf_cr_crlf` and folks have concluded to
> change the rule default behaviour to `lf_cr_crlf`. So, I think it's
> better for us to use their defaults.
>
>
> I've prepared PR [4] with my proposal and tested it on TC and locally
> (on Windows). Most of the changes related to EmptyLineSeparator rule
> which has been fixed here (EmptyLineSeparator check does not validate
> newlines before comments. [6]) and included to 8.20 release version.
>
> Please, consider applying PR [4].
> Also, let's add a recommendation to configure `autocrlf` option for git.
>
> [1] https://issues.apache.org/jira/browse/MCHECKSTYLE-376
> [2] https://checkstyle.org/releasenotes.html#Release_8.21
> [3] https://github.com/checkstyle/checkstyle/issues/4073
> [4] https://github.com/apache/ignite/pull/7174
> [5] https://issues.apache.org/jira/browse/IGNITE-12472
> [6] https://github.com/checkstyle/checkstyle/issues/5981
> [7] https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration
>
> On Thu, 19 Dec 2019 at 20:55, Andrey Gura <[hidden email]> wrote:
> >
> > Agree with Pavel. But it could broke something in our pipeline.
> >
> > Maxim, if issue was fixed so it dependency update is better way. But
> > it seems that this issue in Open status.
> >
> > On Thu, Dec 19, 2019 at 7:22 PM Pavel Tupitsyn <[hidden email]> wrote:
> > >
> > > Maxim,
> > >
> > > The guidelines are not set in stone.
> > > If we decide that some guideline does not bring any value and only wastes
> > > our time (like this one),
> > > we can (and should) remove it.
> > >
> > > On Thu, Dec 19, 2019 at 7:13 PM Maxim Muzafarov <[hidden email]> wrote:
> > >
> > > > Pavel,
> > > >
> > > > It's configured according to accepted Coding Guidelines [1].
> > > >
> > > > [1]
> > > > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Whitespacesandemptylines
> > > >
> > > > On Thu, 19 Dec 2019 at 18:59, Pavel Tupitsyn <[hidden email]> wrote:
> > > > >
> > > > > Igniters,
> > > > >
> > > > > Does this rule bring any value whatsoever for us?
> > > > > Let's just disable it.
> > > > >
> > > > > On Thu, Dec 19, 2019 at 6:25 PM Maxim Muzafarov <[hidden email]>
> > > > wrote:
> > > > >
> > > > > > Hello, Andrey
> > > > > >
> > > > > > Is it better to upgrade the checkstyle plugin version?
> > > > > > It seems the issue has been fixed since 8.21 version (currently we have
> > > > > > 8.19)
> > > > > >
> > > > > > [1] https://checkstyle.org/releasenotes.html#Release_8.21
> > > > > >
> > > > > > On Thu, 19 Dec 2019 at 18:09, Andrey Gura <[hidden email]> wrote:
> > > > > > >
> > > > > > > Igniters,
> > > > > > >
> > > > > > > recently I run build with checkstyle profile on Windows machine and
> > > > > > > got 8 issues related to the "NewlineAtEndOfFile" rule while there are
> > > > > > > no problems on my Linux machine.
> > > > > > >
> > > > > > > I investigated the problem and suggest explicitly configure this rule
> > > > > > > by LF separator. See [1] for more details.
> > > > > > >
> > > > > > > I want merge this change if there are no any objections from
> > > > community
> > > > > > members.
> > > > > > >
> > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-12472
> > > > > >
> > > >

--
Best regards,
Ivan Pavlukhin