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