ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vyacheslav Daradur <daradu...@gmail.com>
Subject Re: Code inspection
Date Tue, 23 Apr 2019 08:53:02 GMT
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