ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Nikolay Izhikov <nizhi...@apache.org>
Subject Re: Code inspection
Date Thu, 14 Feb 2019 13:08:08 GMT
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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message