Cos, your language seems too harsh for the situation. No one here is committing negligence. The explanation is simple: people aren't perfect.
Now, let's take a step back and see the big picture. Around 95% of the commits in this project are by GridGain personnel (check git shortlog -s -n) who have spent months/years working on this codebase during their daily job. Their eyes are accustomed to this code style and naturally they'll spot oddities in a twitch. It's obvious. For newer people, we don't even have checkstyle nor decent facilities for newer people to spot formatting issues quickly. Because, surprise! The issues that Yakov spotted are simply of formatting. The code is functional and much better tested than other streamers and IP Finders. Other streamers have 1 test, this streamer has 9 unit tests! Look at the code. Furthermore, Yakov seems to have made a mistake reading the Git commit history. There were never WIP commits on master. So may I ask you to stop using catastrophic vocabulary. The situation is not catastrophic, it's simply improvable. Now, as an ASF member, I ask you to recognise that unaffiliated volunteers like me bring diversity to the project that's otherwise dominated by a company. You should appreciate that – more so given that you're a former mentor. I do this for the fun, and attacks like yours take the fun out of it. Have a look again at this project's team composition and, for those people not affiliated to GridGain, try to find when their last commit was... Then you'll see what I mean. P.S.: I did not merge the ZK IP Finder myself and I'm assuming that Valentin will want to comment. Regards, *Raúl Kripalani* PMC & Committer @ Apache Ignite, Apache Camel | Integration, Big Data and Messaging Engineer http://about.me/raulkripalani | http://www.linkedin.com/in/raulkripalani http://blog.raulkr.net | twitter: @raulvk 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 >
