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 Thu, 14 Feb 2019 13:21:17 GMT
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

Mime
View raw message