Ivan, As the first implementation of this addition, I'd prefer to make it working like _Licenses Headers_ suite check. It will fail when some of the code style checks violated. Moreover, these licenses header checks can be included in the checkstyle plugin configuration.
In general, I'd prefer to have a compilation fail error with code style checks and after we will get a stable checkstyle suite I propose to change it in a "compilation error" way. If we are talking about the coding style convenient for most of the community members I see no difference with coding sketches or production-ready branches equally. Indeed, no one will be against unused imports [or spaces instead of tabs :-) ] in their PRs or prototypes, right? (for instance, it can be automatically removed by IDE at commit phase). Please, note currently enabled checks are: - list.isEmpty() instead of list.size() == 0 - unused imports - missing @Override - sotred modifiers checks (e.g. pulic static final ..) - redundunt suppersion checks - spaces insted of tabs. Are you really what to violate these checks in your sketches? Hope not :-) On Wed, 13 Feb 2019 at 10:25, Nikolay Izhikov <nizhi...@apache.org> wrote: > > Actually, I dont see anything wrong with failing *compilation* task. > > I think one should use project code style for everyday coding, not only for > ready-to-merge PRs. > > If we cant use code style for everyday coding, we should change the > codestyle. > > What do you think? > > ср, 13 февр. 2019 г., 10:11 Petr Ivanov mr.wei...@gmail.com: > > > I guess that was about failing build configuration with Checkstype, not > > compilation build itself. > > > > > On 12 Feb 2019, at 18:03, Павлухин Иван <vololo...@gmail.com> wrote: > > > > > > Folks, > > > > > > Are you going to fail job compiling Ignite sources [1] if some > > > inspection found a problem? Can we avoid it? It is quite common > > > pattern to start some feature implementation with making a sketch and > > > running tests against it. I found it convenient to skip some style > > > requirements for such sketches (e.g. well formed javadocs). > > > > > > [1] > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_BuildApacheIgnite > > > > > > пн, 11 февр. 2019 г. в 11:38, Nikolay Izhikov <nizhi...@apache.org>: > > >> > > >> Petr, we should have 1 configuration for project, may be 1 configuration > > >> per programming language. > > >> > > >> пн, 11 февр. 2019 г., 11:33 Petr Ivanov mr.wei...@gmail.com: > > >> > > >>> I was asking about how many build configuration is intended? One for > > all > > >>> and multiple per module? > > >>> > > >>> With IDEA inspections it was going to be build configuration per > > module. > > >>> > > >>> > > >>> > > >>> > > >>>> On 11 Feb 2019, at 11:24, Nikolay Izhikov <nizhi...@apache.org> > > wrote: > > >>>> > > >>>> Hello, Petr. > > >>>> > > >>>> Are you saying that we have not single build task? And each module > > builds > > >>>> when it required? If yes, then I propose to create a task like > > "Licence > > >>>> check" which will be run for every patch. > > >>>> > > >>>> My point is that violation of codestyle should be treated as hard as > > >>>> compile error. > > >>>> > > >>>> пн, 11 февр. 2019 г., 11:16 Petr Ivanov mr.wei...@gmail.com: > > >>>> > > >>>>> Is build configuration Inspections [Core] meant to transform into > > single > > >>>>> all-modules check build configuration (without module subdivision)? > > >>>>> > > >>>>> > > >>>>>> On 11 Feb 2019, at 11:02, Nikolay Izhikov <nizhi...@apache.org> > > wrote: > > >>>>>> > > >>>>>> Hello, Maxim. > > >>>>>> > > >>>>>> +1 from me for migrating to checkstyle. > > >>>>>> > > >>>>>> Oleg, there is plugin for IDEA with 2mln downloads - > > >>>>>> https://plugins.jetbrains.com/plugin/1065-checkstyle-idea > > >>>>>> > > >>>>>> I propose do the following: > > >>>>>> > > >>>>>> 1. Migrate current checks to checkstyle. > > >>>>>> 2. Apply checks to all Ignite modules. Currently, only core module > > are > > >>>>>> checked. > > >>>>>> I will review and commit this patch, or do it by my own. > > >>>>>> > > >>>>>> 3. Include code style checks to "Build Apache Ignite" suite. Ignite > > has > > >>>>> to > > >>>>>> fail to build if patch violates codestyle. > > >>>>>> > > >>>>>> вс, 10 февр. 2019 г. в 07:54, Павлухин Иван <vololo...@gmail.com>: > > >>>>>> > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> I also think that some warning from IDEA that some code style rule > > is > > >>>>>>> violated is a must-have. > > >>>>>>> > > >>>>>>> вс, 10 февр. 2019 г. в 01:58, oignatenko <oignate...@gridgain.com > > >: > > >>>>>>>> > > >>>>>>>> Hi Maxim, > > >>>>>>>> > > >>>>>>>> I believe that whatever style checks we establish at Teamcity, we > > >>>>> better > > >>>>>>>> take care of making it easy for developers to find and fix > > violations > > >>>>> in > > >>>>>>>> their typical dev environment (for Ignite this means, in IDEA). I > > >>> think > > >>>>>>> it > > >>>>>>>> is important that developers can maintain required style with > > minimal > > >>>>>>> effort > > >>>>>>>> on their side. > > >>>>>>>> > > >>>>>>>> If above is doable then I am 200% for migrating our Teamcity > > >>>>> inspections > > >>>>>>> to > > >>>>>>>> checkstyle / maven. > > >>>>>>>> > > >>>>>>>> This is because I am very disappointed observing how it stays > > broken > > >>>>> for > > >>>>>>> so > > >>>>>>>> long. And worst of all, even when (if) it is fixed, I feel we will > > >>>>>>> always be > > >>>>>>>> at risk that it breaks again and that we will have to again wait > > for > > >>>>>>> months > > >>>>>>>> for it to be fixed. > > >>>>>>>> > > >>>>>>>> This is such a stark contrast with my experience regarding > > checkstyle > > >>>>>>> based > > >>>>>>>> inspections. These just work and you just never fear that it is > > going > > >>>>> to > > >>>>>>>> break for some obscure reason, this is so much better than what I > > >>>>> observe > > >>>>>>>> now. > > >>>>>>>> > > >>>>>>>> One suggestion in case if we pick checkstyle - I recommend keeping > > >>> its > > >>>>>>>> config file somewhere in the project under version control. I > > used to > > >>>>>>>> maintain such a shared style config at one of past jobs and after > > >>> some > > >>>>>>>> experimenting it turned out most convenient to have it this way - > > so > > >>>>> that > > >>>>>>>> developers could easily assess and discuss style settings and keep > > >>>>> track > > >>>>>>> of > > >>>>>>>> changes in these. (note how Kafka folks from your link [5] appear > > to > > >>> be > > >>>>>>>> doing it this way) > > >>>>>>>> > > >>>>>>>> regards, Oleg > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> Mmuzaf wrote > > >>>>>>>>> Igniters, > > >>>>>>>>> > > >>>>>>>>> I've found that some of the community members have faced with > > >>>>>>>>> `[Inspections] Core suite [1]` is not working well enough on TC. > > The > > >>>>>>>>> suite has a `FAILED` status for more than 2 months due to some > > >>> issues > > >>>>>>>>> in TeamCity application [2]. Current suite behaviour confuses not > > >>> only > > >>>>>>>>> new contributors but also other community members. Moreover, this > > >>>>>>>>> suite is no longer checks rules we previously configured. For > > >>>>>>>>> instance, in the master branch, I've found 11 `Unused imports` > > which > > >>>>>>>>> should have been caught earlier (e.g. for > > >>>>>>>>> {{IgniteCachePutAllRestartTest} [3]). > > >>>>>>>>> > > >>>>>>>>> I think we should make the next step to enable an automatic code > > >>> style > > >>>>>>>>> checks. As an example, we can consider the Apache Kafka code > > style > > >>> [5] > > >>>>>>>>> way and configure for the Ignite project a > > maven-checkstyle-plugin > > >>>>>>>>> with its own maven profile and run it simultaneously with other > > TC. > > >>> We > > >>>>>>>>> can also enable the previously configured inspection rules, so no > > >>>>>>>>> coding style violations will be missed. > > >>>>>>>>> > > >>>>>>>>> I see some advantages of using a maven plugin: > > >>>>>>>>> - an IDE agnostic way for code checks > > >>>>>>>>> - can be used with different CI and build tools (Jenkins, TC) > > >>>>>>>>> - executable from the command line > > >>>>>>>>> - the entry single point to configure new rules > > >>>>>>>>> > > >>>>>>>>> I've created the ticket [4] and will prepare PR for it. > > >>>>>>>>> > > >>>>>>>>> WDYT? > > >>>>>>>>> > > >>>>>>>>> [1] > > >>>>>>>>> > > >>>>>>> > > >>>>> > > >>> > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_InspectionsCore&branch_IgniteTests24Java8=%3Cdefault%3E&tab=buildTypeStatusDiv > > >>>>>>>>> [2] https://youtrack.jetbrains.com/issue/TW-58504 > > >>>>>>>>> [3] > > >>>>>>> > > >>>>> > > >>> > > https://github.com/apache/ignite/blob/master/modules/core/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCachePutAllRestartTest.java#L29 > > >>>>>>>>> [4] https://issues.apache.org/jira/browse/IGNITE-11277 > > >>>>>>>>> [5] https://github.com/apache/kafka/tree/trunk/checkstyle > > >>>>>>>>> > > >>>>>>>>> On Fri, 21 Dec 2018 at 16:03, Petr Ivanov < > > >>>>>>>> > > >>>>>>>>> mr.weider@ > > >>>>>>>> > > >>>>>>>>> > wrote: > > >>>>>>>>>> > > >>>>>>>>>> It seems there is bug in latest 2018.2 TeamCity > > >>>>>>>>>> Bug is filed [1] > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> [1] https://youtrack.jetbrains.com/issue/TW-58504 > > >>>>>>>>>> > > >>>>>>>>>>> On 19 Dec 2018, at 11:31, Petr Ivanov < > > >>>>>>>> > > >>>>>>>>> mr.weider@ > > >>>>>>>> > > >>>>>>>>> > wrote: > > >>>>>>>>>>> > > >>>>>>>>>>> Investigating problem, stand by. > > >>>>>>>>>>> > > >>>>>>>>>>> > > >>>>>>>>>>>> On 18 Dec 2018, at 19:41, Dmitriy Pavlov < > > >>>>>>>> > > >>>>>>>>> dpavlov@ > > >>>>>>>> > > >>>>>>>>> > wrote: > > >>>>>>>>>>>> > > >>>>>>>>>>>> Both patches were applied. Maxim, thank you! > > >>>>>>>>>>>> > > >>>>>>>>>>>> What about 1. An `Unexpected error during build messages > > >>>>>>> processing in > > >>>>>>>>>>>> TeamCity`, what can we do as the next step to fix it? > > >>>>>>>>>>>> > > >>>>>>>>>>>> Sincerely, > > >>>>>>>>>>>> Dmitriy Pavlov > > >>>>>>>>>>>> [cut] > > >>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> -- > > >>>>>>>> Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> -- > > >>>>>>> Best regards, > > >>>>>>> Ivan Pavlukhin > > >>>>>>> > > >>>>> > > >>>>> > > >>> > > >>> > > > > > > > > > > > > -- > > > Best regards, > > > Ivan Pavlukhin > > > >
