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 12:21:06 GMT
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
View raw message