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 <ra...@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 <c...@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
>
