ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sergi Vladykin <sergi.vlady...@gmail.com>
Subject Re: Coding Guidelines: zookeeper IP finder and mqtt streamer
Date Mon, 28 Sep 2015 13:52:05 GMT
Agree about checkstyle. I myself do mistakes even after more than 3 years
on this code base.
Checkstyle is a nice tool, works for many IDEs. For example H2 database
requires
incoming patches to conform provided checkstyle config, I use it right from
Eclipse.

Sergi

2015-09-28 16:25 GMT+03:00 Raul Kripalani <raulk@apache.org>:

> Hey guys,
>
> I have little time now, but before this discussion escalates...
>
> #1 Yakov, how exactly are you reading the Git history? WIP commits were
> never on master, they were in a feature branch which was merged into master
> on this commit:
>
>     Commit: 88acd318b84ce3bff8c061bb34718e0e5f7127fb [88acd31]
>     Parents: 421a5234b5, 296dd6e7d8
>     Author: Raul Kripalani <raulk@apache.org>
>     Date: 21 Sep 2015 17:26:04 WEST
>
>     IGNITE-535 Merge MQTT Streamer into master.
>
>
> Screenshot of the graph here: https://imgur.com/6vy0Vc4.
>
> #2 About TC tests, please see this discussion:
>
> http://apache-ignite-developers.2346864.n4.nabble.com/Maybe-review-my-first-commits-td3370.html
> .
>
> #3 About formatting: yes there may be some problems. To the trained eye
> they are very easy to spot, especially if you spend most of your time in
> the Ignite codebase. But as more people join the team from other walks of
> life, we need a better system:
>
>   (a) the Coding Guidelines are not meticulous enough for the level of
> auditing the community is doing; I get the impression that subjective bias
> is playing a part here, e.g. the "logical blocks" comment -- can you
> provide an objective definition accepted by industry?
>
>   (b) we have no tooling; only a IntelliJ formatting definition, which on
> the one hand is incoherent (e.g. it tries to turn single-line Javadoc into
> multiline) and secondly, it is only valid for IntelliJ users (aren't there
> more IDEs out there?).
>
>   (c) since we have no tooling in place, you are expecting a human to
> review every single character meticulously; this is inefficient and
> discourages people from contributing.
>
>   (d) for the people who are so concerned with this level of detail, would
> you consider writing a checkstyle definition? It'll provide an objective
> basis, and we can plug it into the build and make it fail. That'll be the
> end of the story ;-)
>
> By the way, I'm sure you are reviewing outdated code because Joiner *is*
> used.
>
> #4 While we are on this, can we please discuss stuff like this:
> https://imgur.com/991Aa4X. Why doesn't the team use the --rebase option
> when pulling? Also, we need to remember that branch names disappear when
> the branch is deleted, therefore the commits lose their context (ticket,
> feature, etc.) if the message does not carry it. You can see how the commit
> log looks to another person in the screenshot.
>
> P.S.: Will reply to your individual formatting comments later, but retrier
> vs retryier is intentional. Retryier is the library (wrong spelling),
> retrier are my variable names (correct spelling).
>
> Regards,
> Raúl.
>
> On Mon, Sep 28, 2015 at 1:53 PM, Konstantin Boudnik <cos@apache.org>
> wrote:
>
> > Are these official guidelines that are worked-out and communicated by
> > community? Basically, were they made clear when the project went on the
> CTR
> > model? I presume it is/was looking at the wikipage. Hence non-sticking
> > to them is a case of negligence and needs to be addressed accordingly.
> >
> > I would also want to highlight the other side of such negligence: by
> > dumping
> > semi-baked code to the master one creates a burden for the rest of the
> > community as the code degrades in quality, potentially breaks tests,
> style
> > checks, etc. And someone else needs to deal with it to unblock her's
> future
> > progress. And that's brings forward another point that Brane and I were
> > making on a few occasions: in the CTR communities you need to invite in
> > people
> > with great deal of attention to how they work with others. Are they
> > respecting
> > others' time and effort? Are they good citizens of the community? And on,
> > and
> > on.
> >
> > Another purely technically matter: master isn't a trash can. Master
> should
> > be
> > close to releasable at any given point of time. WIP stuff doesn't belong
> to
> > master, that's what the dev and integration branches are for.
> >
> > Cos
> >
> > On Mon, Sep 28, 2015 at 03:31PM, Yakov Zhdanov wrote:
> > > Guys,
> > >
> > > I have just reviewed the code and have some comments. 1-2 are very
> > serious
> > > from my point of view.
> > >
> > > 1. Code is in master. Did anyone checked tests on TC? Moreover, are
> there
> > > suites for those tests?
> > > 2. It seems that work on streamer has been done directly in master. I
> see
> > > WIP commits, but I think I should not. As agreed finished work should
> be
> > > committed as a single set of changes.
> > > 3. I see unused variable
> > > - org.apache.ignite.stream.mqtt.MqttStreamer#cachedLogPrefix
> > > 4. Unused import - import com.google.common.base.Joiner;
> > > 5. Code and javadocs lines exceed 120 chars restriction.
> > > 6. Plenty of javadocs issues - absence, multiline "inheritdoc", etc.
> > > 7. Spacing is not correct - in ignite codebase logical blocks are
> > separated
> > > with blank line.
> > > 8. There should always be a blank line at the end of each file.
> > > 9. retrier vs retryier issue.
> > >
> > > Who is in charge for this code? Raul, Val? Can anyone fix my comments?
> > >
> > > I would also ask everyone (even committers) not to commit to master
> > without
> > > doing review with another committer.
> > >
> > > Here is the link to Ignite's coding guidelines -
> > > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines.
> > Feel
> > > free to suggest and discuss edits if anything does not seem valid to
> you.
> > >
> > > Thanks!
> > >
> > > --Yakov
> >
>

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