ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Petr Ivanov <mr.wei...@gmail.com>
Subject Re: Code inspection
Date Tue, 23 Apr 2019 09:26:08 GMT
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.weider@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 <daradurvs@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.weider@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 <daradurvs@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 <daradurvs@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 <maxmuzaf@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 <daradurvs@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 <maxmuzaf@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 <dpavlov@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, Павлухин Иван <vololo100@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, Павлухин Иван <vololo100@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 <nizhikov@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 <maxmuzaf@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, Павлухин Иван <vololo100@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 <maxmuzaf@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 Павлухин Иван <vololo100@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 <maxmuzaf@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, Павлухин Иван <
>>> 
>>> vololo100@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 <
>>> 
>>> nizhikov@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, Павлухин Иван <
>>> 
>>> vololo100@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 <
>>> 
>>> nizhikov@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, Павлухин Иван <
>>> 
>>> vololo100@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 <
>>> 
>>> maxmuzaf@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 <
>>> 
>>> nizhikov@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.weider@gmail.com:
>>> 
>>> 
>>> I guess that was about failing build
>>> 
>>> configuration with
>>> 
>>> Checkstype,
>>> 
>>> not
>>> 
>>> compilation build itself.
>>> 
>>> On 12 Feb 2019, at 18:03, Павлухин Иван <
>>> 
>>> vololo100@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 <
>>> 
>>> nizhikov@apache.org
>>> 
>>> :
>>> 
>>> 
>>> Petr, we should have 1 configuration for
>>> 
>>> project, may be 1
>>> 
>>> configuration
>>> 
>>> per programming language.
>>> 
>>> пн, 11 февр. 2019 г., 11:33 Petr Ivanov
>>> 
>>> mr.weider@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
>>> 
>>> <
>>> 
>>> nizhikov@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.weider@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 <
>>> 
>>> nizhikov@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, Павлухин
>>> 
>>> Иван <
>>> 
>>> vololo100@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 <
>>> 
>>> oignatenko@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 &lt;
>>> 
>>> 
>>> mr.weider@
>>> 
>>> 
>>> &gt; 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 &lt;
>>> 
>>> 
>>> mr.weider@
>>> 
>>> 
>>> &gt; wrote:
>>> 
>>> 
>>> Investigating problem, stand by.
>>> 
>>> 
>>> On 18 Dec 2018, at 19:41, Dmitriy
>>> 
>>> Pavlov &lt;
>>> 
>>> 
>>> dpavlov@
>>> 
>>> 
>>> &gt; 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.
> 


Mime
View raw message