ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Code inspection
Date Tue, 12 Feb 2019 15:03:21 GMT
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

Mime
View raw message