Folks, In addition to answers above, I think TC.Bot must comment PR with violated checkstyle rules and references to source code if the build has failed. This may be more convenient for contributors from my point of view.
On Tue, 7 Jul 2020 at 11:50, Ivan Pavlukhin <[email protected]> wrote: > > Folks, > > This discussion reoccurs from month to month. Last one I remember is > [1]. It sounds useful to pin a decision to our coding guidelines. What > do you think? > > [1] > http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html > > 2020-07-07 11:28 GMT+03:00, Вячеслав Коптилин <[email protected]>: > > Nikolay, > > > > There is *NO *intention to ignore code style violations and do merge PRs > > into the master branch without fixing them. > > > > Let's assume that I want to implement a dirty fix of a bug, check a > > reproducer from the user list, etc. > > And I do not have the intention to merge that into the master branch as is, > > however, I do not want to waste my time fixing all code style violations. > > Does this use-case look reasonable? > > > > Thanks, > > Slava. > > > > вт, 7 июл. 2020 г. в 11:07, Nikolay Izhikov <[email protected]>: > > > >> Slava. > >> > >> All contributors should follow our code style during their contribution. > >> For now, we have an automatic PR check that is integrated to the GitHub > >> interface. > >> > >> I don’t see any issue here. > >> All open-source project that I know, uses the same approach. > >> > >> Anyway, If some of the experienced community members is interested in the > >> particular contribution he or she can help a newcomer with the code style > >> - > >> GitHub interface has the edit button even for someone else’s PR’s > >> > >> > >> > 7 июля 2020 г., в 11:01, Вячеслав Коптилин <[email protected]> > >> написал(а): > >> > > >> > Hello Nikolay, > >> > > >> >> Because any code style violations should be fixed before the merge. > >> >> Therefore after the fix, you must rerun TC. > >> >> This means that running heavy suites when code style is violated is a > >> > waste of the resources. > >> > This makes sense, however, to be honest, I don't see that our team city > >> > servers are really busy. > >> > > >> >> Why do you want to violate code style rules in your PR? > >> > Please take a look at the original e-mail from Ilya: > >> >> This means that I have completely lost an option to run tests against > >> > pull > >> >> requests by new contributors - they usually compile but will not pass > >> >> Checkstyle. That's a blocker. > >> > > >> > This issue has also been discussed here: > >> > > >> http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Separate-code-sanity-check-and-build-task-td47003.html > >> > > >> > Thanks, > >> > Slava. > >> > > >> > > >> > вт, 7 июл. 2020 г. в 10:47, Nikolay Izhikov <[email protected]>: > >> > > >> >> All checks just force rules we agreed on. > >> >> > >> >>> Why this check is so important? Why do you think it is more important > >> >> than all other tasks/tests? > >> >> > >> >> Because any code style violations should be fixed before the merge. > >> >> Therefore after the fix, you must rerun TC. > >> >> This means that running heavy suites when code style is violated is a > >> >> waste of the resources. > >> >> > >> >> Why do you want to violate code style rules in your PR? > >> >> > >> >> I think instead of hiding style errors we should make our code style > >> >> comfortable for everyone. > >> >> Can you, please, write - what part of the code style hurts you? > >> >> > >> >> > >> >>> 7 июля 2020 г., в 10:39, Вячеслав Коптилин <[email protected]> > >> >> написал(а): > >> >>> > >> >>> Hello Maxim, > >> >>> > >> >>>> Why do you think that splitting the checkstyle build is better > >> >>>> option > >> >>> than fixing code style issues reporting by the checkstyle plugin? > >> >>> Why do you think that Ilya talks that code style errors should not be > >> >> fixed? > >> >>> > >> >>> It looks weird to me that we do not even start the tests if check > >> >>> style > >> >>> plugin reports violations. > >> >>> Why can't this check be done in parallel with the tests and reported > >> >>> by > >> >>> mtcga bot? > >> >>> Why this check is so important? Why do you think it is more important > >> >> than > >> >>> all other tasks/tests? > >> >>> > >> >>> Thanks, > >> >>> Slava. > >> >>> > >> >>> пн, 6 июл. 2020 г. в 20:34, Maxim Muzafarov <[email protected]>: > >> >>> > >> >>>> Hello Ilya, > >> >>>> > >> >>>> Why do you think that splitting the checkstyle build is better > >> >>>> option > >> >>>> than fixing code style issues reporting by the checkstyle plugin? > >> >>>> > >> >>>> On Mon, 6 Jul 2020 at 19:43, Ilya Kasnacheev < > >> [email protected] > >> >>> > >> >>>> wrote: > >> >>>>> > >> >>>>> Hello! > >> >>>>> > >> >>>>> I have just noticed today that Checkstyle will fail Apache Ignite > >> >> build: > >> >>>>> > >> >>>> > >> >> > >> https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_BuildApacheIgnite/5443282?buildTab=log&focusLine=3&linesState=683.4289 > >> >>>>> > >> >>>>> This means that I have completely lost an option to run tests > >> >>>>> against > >> >>>> pull > >> >>>>> requests by new contributors - they usually compile but will not > >> >>>>> pass > >> >>>>> Checkstyle. That's a blocker. > >> >>>>> > >> >>>>> Can we please split Checkstyle as a separate build which is > >> >>>>> triggered > >> >>>> with > >> >>>>> Run All? > >> >>>>> I think we even have > >> >>>>> > >> >>>> > >> >> > >> https://ci.ignite.apache.org/buildConfiguration/IgniteTests24Java8_CheckCodeStyle?mode=builds#all-projects > >> >>>>> > >> >>>>> WDYT? > >> >>>>> > >> >>>>> Regards, > >> >>>>> -- > >> >>>>> Ilya Kasnacheev > >> >>>> > >> >> > >> >> > >> > >> > > > > > -- > > Best regards, > Ivan Pavlukhin
