Hello, Petr. The main issue here is - we, as a community, still think that issue with code style is "minor". Personally, I think there is no "minor" issues with code style - we should force single code style as hard as we can.
I think we *must* fail entire build if there are code style issues. В Вт, 23/04/2019 в 13:03 +0300, Vyacheslav Daradur пишет: > > > Also I still strictly against adding checkstyle to project build as minor > > > issues in checkstyle should not be blocker for test run. > > Me too. > > Looks like everything works fine. Thanks! The only problem is long > time of build ~10 minutes, because of dependencies resolving. > > Maxim, please double check it. > > On Tue, Apr 23, 2019 at 12:26 PM Petr Ivanov <mr.wei...@gmail.com> wrote: > > > > Vyacheslav, can you check this build please [1] if everything was ran as > > expected? > > > > Also I still strictly against adding checkstyle to project build as minor > > issues in checkstyle should not be blocker for test run. > > > > > > [1] > > https://ci.ignite.apache.org/viewLog.html?buildId=3678000&buildTypeId=IgniteTests24Java8_CheckCodeStyle&tab=artifacts&branch_IgniteTests24Java8=%3Cdefault%3E# > > > > > > > On 23 Apr 2019, at 11:59, Petr Ivanov <mr.wei...@gmail.com> wrote: > > > > > > I'll check it. > > > > > > > > > Also, please pass TC build for review next time and do not add to Run All > > > without it. > > > > > > Thanks! > > > > > > > > > > On 23 Apr 2019, at 11:53, Vyacheslav Daradur <daradu...@gmail.com> > > > > wrote: > > > > > > > > This is quite strange error, in case of "install" phase it'd be better > > > > just add "checkstyle" profile to "Build" [1] configuration. > > > > > > > > [1] > > > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_BuildApacheIgnite > > > > > > > > On Tue, Apr 23, 2019 at 11:43 AM Petr Ivanov <mr.wei...@gmail.com> > > > > wrote: > > > > > > > > > > The suite is malformed. > > > > > If no ~/.m2/repository/org/apache/ignite artifact are installed in > > > > > system, the build will fail [1] > > > > > > > > > > It seems that we should use install instead of validate. > > > > > > > > > > > > > > > [1] > > > > > https://ci.ignite.apache.org/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=3677858&_focus=288 > > > > > > > > > > > > > > > On 23 Apr 2019, at 00:25, Vyacheslav Daradur <daradu...@gmail.com> > > > > > wrote: > > > > > > > > > > Maxim, I merged your changes to master. > > > > > > > > > > Also, I've created a new build plan "Check Code Style" on TC [1] and > > > > > included in RunAll build. > > > > > The report of check-style attaches in artifacts once build is > > > > > finished. > > > > > > > > > > Please check that it works as expected once again and announce new > > > > > requirements in a separate thread to avoid confusion of community. > > > > > > > > > > cc Petr, Pavel (JFYI) > > > > > > > > > > [1] > > > > > https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_CheckCodeStyle&tab=buildTypeBranches > > > > > > > > > > On Sun, Apr 21, 2019 at 10:18 PM Vyacheslav Daradur > > > > > <daradu...@gmail.com> wrote: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > I left some comments in Jira, let's resolve them and I'll assist you > > > > > with merge and TC configuring. > > > > > > > > > > On Fri, Apr 19, 2019 at 5:50 PM Maxim Muzafarov <maxmu...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > Vyacheslav, > > > > > > > > > > Thank you for your interest in making Ignite coding style better. > > > > > > > > > > The short answer is - there are no different checkstyle > > > > > configurations. One for the JetBrains Inspections, and one the > > > > > Checkstyle plugin. This is a completely different approach for > > > > > checking the Ignites source code. > > > > > > > > > > Currently, we have two different configurations for the JetBrains IDEA > > > > > Inspection check: > > > > > - ignite\.idea\inspectionProfiles\Project_Default.xml - this is > > > > > default on the IDE level and used silently by every developer whos > > > > > checkout Ignite project (it remains) > > > > > - ignite\idea\ignite_inspections_teamcity.xml - this is the > > > > > configuration of the inspection for the TC suite (it will be deleted) > > > > > It's unobvious to maintain both of them. Previously we've planned to > > > > > fix all the inspection rules one by one and add them one by one to the > > > > > TC inspection configuration file (something like storing the > > > > > intermediate result), but it didn't happen cause the inspection TC > > > > > suite got broken after migration to 2018 version. > > > > > > > > > > Now it seems to me, that it is better to use the best open source > > > > > practices like checkstyle plugin (380K usages on github repositories) > > > > > rather than proprietary software. So, we will: > > > > > - keep IDE level inspection configuration (the Project_Default.xml > > > > > config); > > > > > - add a new checkstyle plugin configuration file (checkstyle.xml > > > > > config) which will be used simultaneously for checking code style on > > > > > build procedure and for the IDE-checkstyle plugin; > > > > > > > > > > On Wed, 17 Apr 2019 at 21:23, Vyacheslav Daradur > > > > > <daradu...@gmail.com> wrote: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > I looked through the PR and it looks good to me in general. > > > > > > > > > > The only question how it's planned to maintain check styles in 2 > > > > > different configurations, for IDEA and check style plugin? > > > > > > > > > > On Mon, Mar 25, 2019 at 12:30 PM Maxim Muzafarov <maxmu...@gmail.com> > > > > > wrote: > > > > > > > > > > > > > > > Igniters, > > > > > > > > > > The issue [1] with enabled maven-checkstyle-plugin is ready for the > > > > > review. > > > > > All changes are prepared according to e-mail [2] the second option > > > > > point (include the plugin in the maven build procedure by default). > > > > > > > > > > JIRA: IGNITE-11277 [1] > > > > > PR: [3] > > > > > Upsource: [4] > > > > > > > > > > How can take a look? > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-11277 > > > > > [2] > > > > > http://apache-ignite-developers.2346864.n4.nabble.com/Code-inspection-tp27709p41297.html > > > > > [3] https://github.com/apache/ignite/pull/6119 > > > > > [4] https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1018 > > > > > > > > > > On Fri, 15 Mar 2019 at 19:19, Dmitriy Pavlov <dpav...@apache.org> > > > > > wrote: > > > > > > > > > > > > > > > Hi Igniters, > > > > > > > > > > I see that a new TeamCity is released: Version: 2018.2.3. > > > > > > > > > > Probably it could solve recently introduced problem related to: > > > > > Unexpected error during build messages processing in TeamCity; > > > > > > > > > > Peter I., could you please check? > > > > > > > > > > Sincerely, > > > > > Dmitriy Pavlov > > > > > > > > > > пт, 15 мар. 2019 г. в 12:04, Павлухин Иван <vololo...@gmail.com>: > > > > > > > > > > I agree to gather some votes according to Maxim's proposal. > > > > > > > > > > Personally, I will not put my vote here. Both options will work for me > > > > > today. > > > > > > > > > > But I would like to say a bit about agility. As I said both options > > > > > sounds fine for me today. And I believe that we can switch from one to > > > > > another easily in future. Let's do our best to be flexible. > > > > > > > > > > пт, 15 мар. 2019 г. в 12:04, Павлухин Иван <vololo...@gmail.com>: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > As far as I understand your case, you want to fix all code styles > > > > > > > > > > issues right before getting the final TC results. Right? ... > > > > > > > > > > Actually, I mostly worry about accidental failures. For simple tasks > > > > > my workflow looks like: > > > > > 1. Create a branch. > > > > > 2. Write some code lines and tests. > > > > > 3. Run the most closely related tests from IDEA. > > > > > 4. Push changes to the branch. > > > > > 5. Launch TC. > > > > > 6. Take a cup of coffee ;-) > > > > > 7. Check TC results after a couple of hours. > > > > > > > > > > And in such workflow I can accidentally leave styling error (IDEA does > > > > > not fail compilation). And I will receive not very valuable report > > > > > from TC. And will have to wait for another couple of hours. > > > > > > > > > > Yes, usually I do not execute "mvn clean install" locally before > > > > > triggering TC. And I think that generally we should not do it because > > > > > TC does it. > > > > > > > > > > If not everybody uses a bot visas it sounds bad for me. For patches > > > > > touching the code it should be mandatory. Also, as you might know > > > > > there are different kind of visas and for some trivial patches we can > > > > > request Checkstyle visa. Committers should check formalities. > > > > > > > > > > пт, 15 мар. 2019 г. в 10:29, Nikolay Izhikov <nizhi...@apache.org>: > > > > > > > > > > > > > > > +1 to enable code style checks in compile time. > > > > > > > > > > We can add option to disable maven codestyle profile with some > > > > > > > > > > environment variable. > > > > > > > > > > > > > > > Anyone who want violate common project rules in their local branch can > > > > > > > > > > set this variable and write some nasty code before push :) > > > > > > > > > > > > > > > пт, 15 марта 2019 г., 9:40 Maxim Muzafarov <maxmu...@gmail.com>: > > > > > > > > > > > > > > > Ivan, > > > > > > > > > > 1. I can write code and execute tests successfully even if there are > > > > > > > > > > some style problems. I can imagine that a style error can arise > > > > > occasionally. And instead of getting test results after several hours > > > > > I will get a build failure without any tests run. I will simply lose > > > > > my time. But if the tests are allowed to proceed I will get a red flag > > > > > from code style check, fix those issues and rerun style check. > > > > > > > > > > As far as I understand your case, you want to fix all code styles > > > > > issues right before getting the final TC results. Right? It's doable > > > > > you can disable checkstyle in your local branch and revet it back when > > > > > you've done with all your changes to get the final visa. But the > > > > > common case here is building the project locally and checking all > > > > > requirements for PR right before pushing it to the GitHub repo. I > > > > > always do so. The "Checklist before push" [1] have such > > > > > recommendations. Build the project before push will eliminate your use > > > > > case. > > > > > > > > > > --- > > > > > > > > > > Igniters, > > > > > > > > > > To summarize the options we have with code checking behaviour: > > > > > > > > > > 1) (code style will be broken more often) Run checkstyle in the > > > > > separate TC suite and include it to the Bot visa. > > > > > - not all of us run TC for their branches especially for simple fixes > > > > > (it's the most common case when a new check style errors occur) > > > > > - not all of us use TC.Bot visa to verify their branches > > > > > - if this checkstyle suite starts to fail it will be ignored for some > > > > > time (not blocks the development process) > > > > > - a lot of suites for code checking (license, checkstyle, something > > > > > else in future) > > > > > > > > > > + a bit comfortable way of TC tests execution for local\prototyped PRs > > > > > (it's a matter of taste) > > > > > + build the project and execute test suites a bit earlier (checkstyle > > > > > on the separate suite does not affect other suites) > > > > > > > > > > 2) (code style will be broken less often) Run checkstyle on project > > > > > > > > > > build stage. > > > > > > > > > > - increases a bit the build time procedure > > > > > - require additional operations to switch it off for prototyping > > > > > > > > > > branches > > > > > > > > > > > > > > > + do not require TC.Bot visa if someone of the community doesn't use > > > > > > > > > > it > > > > > > > > > > + code style errors will be fixed immediately if the master branch > > > > > starts to fail > > > > > + the single place for code checks on maven code validation stage > > > > > (license check suite can be removed) > > > > > > > > > > > > > > > Please, add another advantages\disadvantages that I've missed. > > > > > Let's vote and pick the most suitable option for the Apache Ignite > > > > > > > > > > needs. > > > > > > > > > > > > > > > --- > > > > > > > > > > Personally, I'd like to choose the 2) option. > > > > > > > > > > The JIRA [2] and PR [3] with the checkstyle enabled plugin is ready > > > > > for the review. > > > > > > > > > > [1] > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-Checklistbeforepush > > > > > > > > > > [2] https://issues.apache.org/jira/browse/IGNITE-11277 > > > > > [3] https://github.com/apache/ignite/pull/6119 > > > > > > > > > > On Thu, 7 Mar 2019 at 11:19, Павлухин Иван <vololo...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > From use cases described by you I use only 1 and 2. And actually I > > > > > think that we can concentrate on 1 and forget about others for now. > > > > > But please address my worries from previous letter: > > > > > ====Quoted text==== > > > > > 1. I can write code and execute tests successfully even if there are > > > > > some style problems. I can imagine that a style error can arise > > > > > occasionally. And instead of getting test results after several > > > > > > > > > > hours > > > > > > > > > > I will get a build failure without any tests run. I will simply lose > > > > > my time. But if the tests are allowed to proceed I will get a red > > > > > > > > > > flag > > > > > > > > > > from code style check, fix those issues and rerun style check. > > > > > 2. Style check takes some time. With simple checks it is almost > > > > > negligible. But it can grow if more checks are involved. > > > > > ====End of quoted text==== > > > > > > > > > > Some clarifications. 1 is about running from IDEA first. 2 is > > > > > > > > > > related > > > > > > > > > > to opinions that we should involve much more checks, e.g. using > > > > > abbreviations. > > > > > > > > > > чт, 7 мар. 2019 г. в 10:36, Maxim Muzafarov <maxmu...@gmail.com>: > > > > > > > > > > > > > > > Ivan, > > > > > > > > > > Let's take a look at all the options we have (ordered by the > > > > > > > > > > frequency of use): > > > > > > > > > > > > > > > 1. Check ready for merge branches (main case) > > > > > 2. Run tests on TC without checkstyle (prototyping branches) > > > > > 3. Local project build > > > > > 4. Quick build without any additional actions on TC > > > > > > > > > > In the other projects (kafka, netty etc.) which I've checked the > > > > > > > > > > checkstyle plugin is used in the <build> section, so the user has no > > > > > chance > > > > > in common cases to disable it via command line (correct me if I'm > > > > > wrong). > > > > > In the PR [1] I've moved checkstyle configuration to the separate > > > > > profile. > > > > > I've set activation checkstyle profile if -DskipTests specified, so > > > > > it will > > > > > run with the basic build configuration. It can also be disabled > > > > > locally if > > > > > we really need it. > > > > > > > > > > > > > > > Back to our use cases: > > > > > > > > > > 1. For checking the ready to merge branches we will fail the > > > > > > > > > > ~Build Apache Ignite~ suite, so no configured checkstyle rules will be > > > > > violated. If we will use the TC.Bot approach someone will merge the > > > > > branch > > > > > without running TC.Bot on it, but no one will merge the branch with > > > > > compile > > > > > errors. > > > > > > > > > > > > > > > 2. For the prototyping branches, you can turn off checkstyle in > > > > > > > > > > your local PR by removing activation configuration. It's ok as these > > > > > type > > > > > of branches will never be merged to the master. > > > > > > > > > > > > > > > 3. From my point, local builds should be always run with the > > > > > > > > > > checkstyle enabled profile. The common build action as `mvn clean > > > > > install > > > > > -DskipTests` will activate the profile. > > > > > > > > > > > > > > > 4. The checkstyle profile can be disabled explicitly on TC by > > > > > > > > > > specifying -P !checkstyle option. A don't see any use cases of it, > > > > > but it's > > > > > completely doable. > > > > > > > > > > > > > > > Please, correct me if I've missed something. > > > > > > > > > > I propose to merge PR [1] as it is, with the configured set of > > > > > > > > > > rules. > > > > > > > > > > > > > > > [1] https://github.com/apache/ignite/pull/6119 > > > > > > > > > > On Tue, 5 Mar 2019 at 19:02 Павлухин Иван <vololo...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > I like an idea of being IDE agnostic. I am ok with currently > > > > > > > > > > enabled > > > > > > > > > > checks, they are a must-have in my opinion (even for prototypes). > > > > > > > > > > But I am still do not like an idea of preventing tests execution > > > > > > > > > > if > > > > > > > > > > style check finds a problem. I checked out PR, installed a > > > > > > > > > > plugin and > > > > > > > > > > tried it out. Here are my concerns: > > > > > 1. I can write code and execute tests successfully even if there > > > > > > > > > > are > > > > > > > > > > some style problems. I can imagine that a style error can arise > > > > > occasionally. And instead of getting test results after several > > > > > > > > > > hours > > > > > > > > > > I will get a build failure without any tests run. I will simply > > > > > > > > > > lose > > > > > > > > > > my time. But if the tests are allowed to proceed I will get a > > > > > > > > > > red flag > > > > > > > > > > from code style check, fix those issues and rerun style check. > > > > > 2. Style check takes some time. With simple checks it is almost > > > > > negligible. But it can grow if more checks are involved. > > > > > > > > > > On the bright side I found nice integration of Checkstyle plugin > > > > > > > > > > with > > > > > > > > > > IDEA commit dialog. There is a checkbox "Scan with Checkstyle" > > > > > > > > > > which I > > > > > > > > > > think is quite useful. > > > > > > > > > > пн, 4 мар. 2019 г. в 15:00, Maxim Muzafarov <maxmu...@gmail.com > > > > > > > > > > : > > > > > > > > > > > > > > > Ivan, > > > > > > > > > > I like that Jetbrains inspections are integrated with IDE and > > > > > > > > > > TC out > > > > > > > > > > of the box, but currently, they are working not well enough on > > > > > > > > > > TC. > > > > > > > > > > Actually, they are not checking our source code at all. > > > > > > > > > > Let's try a bit another approach and try to be IDE-agnostic > > > > > > > > > > with code > > > > > > > > > > style checking. I've checked popular java projects: hadoop, > > > > > > > > > > kafka, > > > > > > > > > > spark, hive, netty. All of them are using > > > > > > > > > > maven-checkstyle-plugin in > > > > > > > > > > their <build> section by default, so why don't we? It sounds > > > > > reasonable for me at least to try so. > > > > > > > > > > Can you take a look at my changes below? > > > > > > > > > > > > > > > Igniters, > > > > > > > > > > PR [2] has been prepared. All the details I've mentioned in my > > > > > > > > > > comment > > > > > > > > > > in JIRA [4]. > > > > > Can anyone take a look at my changes? > > > > > > > > > > JIRA: [1] > > > > > PR: [2] > > > > > Upsource: [3] > > > > > > > > > > Questions to discuss: > > > > > 1) There is no analogue for inspections RedundantSuppression > > > > > > > > > > and > > > > > > > > > > SizeReplaceableByIsEmpty (all code style checks [5]). Propose > > > > > > > > > > to merge > > > > > > > > > > without them. > > > > > 2) Checkstyle plugin has it's own maven profile and enabled by > > > > > default. It can be turned off for prototype branches. > > > > > 3) I've removed the inspections configuration for the TC suite > > > > > > > > > > and > > > > > > > > > > propose to disable it as not working. > > > > > > > > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-11277 > > > > > [2] https://github.com/apache/ignite/pull/6119 > > > > > [3] > > > > > > > > > > https://reviews.ignite.apache.org/ignite/review/IGNT-CR-1018 > > > > > > > > > > [4] > > > > > > > > > > https://issues.apache.org/jira/browse/IGNITE-11277?focusedCommentId=16771200&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16771200 > > > > > > > > > > [5] http://checkstyle.sourceforge.net/checks.html > > > > > > > > > > On Thu, 14 Feb 2019 at 16:21, Павлухин Иван < > > > > > > > > > > vololo...@gmail.com> wrote: > > > > > > > > > > > > > > > Nikolay, > > > > > > > > > > All community members are forced to follow code style. > > > > > > > > > > It's harder to achieve it with dedicated suite. > > > > > > > > > > Why it is easier to follow code style with use of maven > > > > > > > > > > checkstyle > > > > > > > > > > plugin? Is it integrated into IDEA out of box? As I got it > > > > > > > > > > additional > > > > > > > > > > IDEA plugin is needed as well. Who will enforce everybody to > > > > > > > > > > install > > > > > > > > > > it? > > > > > > > > > > Also, as I see a common good practice today is using TC Bot > > > > > > > > > > visa. Visa > > > > > > > > > > includes result from running inspections job. > > > > > > > > > > чт, 14 февр. 2019 г. в 16:08, Nikolay Izhikov < > > > > > > > > > > nizhi...@apache.org>: > > > > > > > > > > > > > > > Ivan, > > > > > > > > > > Could you please outline the benefits you see of failing > > > > > > > > > > compilation and > > > > > > > > > > skipping tests execution if inspections detect a problem? > > > > > > > > > > All community members are forced to follow code style. > > > > > It's harder to achieve it with dedicated suite. > > > > > > > > > > > > > > > чт, 14 февр. 2019 г. в 15:21, Павлухин Иван < > > > > > > > > > > vololo...@gmail.com>: > > > > > > > > > > > > > > > Nikolay, > > > > > > > > > > Should the community spend TC resources for prototype? > > > > > > > > > > Why not? I think it is not bad idea to run all tests > > > > > > > > > > against some > > > > > > > > > > changes into core classes. If I have a clever idea which > > > > > > > > > > is easy to > > > > > > > > > > test drive I can do couple of prototype-test iterations. > > > > > > > > > > If tests > > > > > > > > > > shows me that everything is bad then the idea was not so > > > > > > > > > > clever and > > > > > > > > > > easy. But if I was lucky then I should discuss the idea > > > > > > > > > > with other > > > > > > > > > > Igniters. I think it is the cheapest way to check the > > > > > > > > > > idea because the > > > > > > > > > > check is fully automated. Requiring a human feedback is > > > > > > > > > > much more > > > > > > > > > > expensive in my opinion. > > > > > > > > > > But, If our code style is not convinient for every day > > > > > > > > > > coding for many > > > > > > > > > > contributors, should you initiate discussion to change > > > > > > > > > > it? > > > > > > > > > > Generally I am fine with our codestyle requirements. > > > > > > > > > > Also, I would like to keep a focus on the subject. Could > > > > > > > > > > you please > > > > > > > > > > outline the benefits you see of failing compilation and > > > > > > > > > > skipping tests > > > > > > > > > > execution if inspections detect a problem? > > > > > > > > > > чт, 14 февр. 2019 г. в 14:14, Nikolay Izhikov < > > > > > > > > > > nizhi...@apache.org>: > > > > > > > > > > > > > > > Hello, Ivan. > > > > > > > > > > Requirements for a prototype code are not the same > > > > > > > > > > as for a patch ready > > > > > > > > > > to merge > > > > > > > > > > True. > > > > > > > > > > I do not see much need in writing good javadocs for > > > > > > > > > > prototype. > > > > > > > > > > > > > > > We, as a community, can't force you to do it. > > > > > > > > > > Why should I stub it to be able run any build on TC? > > > > > > > > > > > > > > > Should the community spend TC resources for prototype? > > > > > You always can check tests for your prototype locally. > > > > > > > > > > And when it's ready, at least from code style point of > > > > > > > > > > view run it on TC. > > > > > > > > > > > > > > > I, personally, always try to follow project code > > > > > > > > > > style, even for > > > > > > > > > > prototypes. > > > > > > > > > > But, If our code style is not convinient for every day > > > > > > > > > > coding for many > > > > > > > > > > contributors, should you initiate discussion to change > > > > > > > > > > it? > > > > > > > > > > > > > > > > > > > > ср, 13 февр. 2019 г. в 16:45, Павлухин Иван < > > > > > > > > > > vololo...@gmail.com>: > > > > > > > > > > > > > > > Maxim, > > > > > > > > > > Oh, my poor tabs.. Joke. > > > > > > > > > > I am totally ok with currently enabled checks. But I > > > > > > > > > > am mostly > > > > > > > > > > concerned about a general approach. I would like to > > > > > > > > > > outline one thing. > > > > > > > > > > Requirements for a prototype code are not the same > > > > > > > > > > as for a patch > > > > > > > > > > ready to merge (see a little bit more in the end of > > > > > > > > > > that message). > > > > > > > > > > > > > > > We have a document defining code style which every > > > > > > > > > > contributor should > > > > > > > > > > follow [1]. And many points can be checked > > > > > > > > > > automatically. Personally, > > > > > > > > > > I do not see much need in writing good javadocs for > > > > > > > > > > prototype. Why > > > > > > > > > > should I stub it to be able run any build on TC? > > > > > > > > > > Also, we a have a review process which should be > > > > > > > > > > applied to every > > > > > > > > > > patch. Partially it is described in [2]. And due to > > > > > > > > > > this process every > > > > > > > > > > patch should not introduce new failures on TC. So, > > > > > > > > > > the patch should > > > > > > > > > > not be merged if inspections failed. > > > > > > > > > > P.S. Something more about prototypes and production > > > > > > > > > > code. There is a > > > > > > > > > > common bad practice in software engineering. It is > > > > > > > > > > turning prototypes > > > > > > > > > > into production code. Often it is much faster to > > > > > > > > > > create a prototype by > > > > > > > > > > price of violating some rules of writing "clean > > > > > > > > > > code". And often > > > > > > > > > > prototype after successful piloting is turned into > > > > > > > > > > production code. > > > > > > > > > > And it is very easy in practice to keep some pieces > > > > > > > > > > of initially > > > > > > > > > > "dirty" prototype code. I believe human factor plays > > > > > > > > > > a great role > > > > > > > > > > here. How should it be done right then? In my > > > > > > > > > > opinion good production > > > > > > > > > > code should be designed as "good production code" > > > > > > > > > > from the beginning. > > > > > > > > > > So, only ideas are taken from the prototype and a > > > > > > > > > > code is fully > > > > > > > > > > rewritten. > > > > > > > > > > [1] > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines > > > > > > > > > > [2] > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist > > > > > > > > > > > > > > > ср, 13 февр. 2019 г. в 15:05, Maxim Muzafarov < > > > > > > > > > > maxmu...@gmail.com>: > > > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > -- > > > > > -- > > > > > Maxim Muzafarov > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best regards, > > > > > Ivan Pavlukhin > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > -- > > > > Best Regards, Vyacheslav D. > >
signature.asc
Description: This is a digitally signed message part
