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 11:14:39 GMT
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
>

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