ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: Fwd: Coding Guidelines: zookeeper IP finder and mqtt streamer
Date Tue, 29 Sep 2015 00:50:48 GMT
One more point about empty lines. I have reviewed our empty line policy and
I don't think I can do a better job describing it. The explanation is
pretty accurate.

However, the main problem with our empty line policy is that, although the
resulting code looks very neat, the policy is just too complex. It would be
nice if someone with correct code style settings could create a correct
auto-format configuration for IDEA and Eclipse. Any volunteers?

D.

On Tue, Sep 29, 2015 at 2:44 AM, Dmitriy Setrakyan <dsetrakyan@apache.org>
wrote:

>
>
> On Tue, Sep 29, 2015 at 12:49 AM, Dmitriy Setrakyan <
> dsetrakyan@gridgain.com> wrote:
>
>> Cos,
>>
>> I think Raul's point was that coding guidelines are not very clear. I
>> think Raul thought that he was following the coding guidelines. I don't
>> think "negligence" is a fair word to describe this.
>>
>> In my view, we have a couple of omissions in the process on Wiki that
>> need to be spelled out clearly:
>>
>> - semantic blocks should be described better in the coding guidelines
>> - we should clearly state that master should always be release-ready in
>> the lira process
>> - the best practice is to go through review in the ticket ignite-1234
>> branch, instead of in master.
>>
>> I will try to fix it and send it here for review.
>>
>
> Guys, I think we have a very confusing Wiki. We have GIT Process page and
> Jira Process pages. Both of them describe some part of the same process. I
> think they should be combined into 1 page with GIT and JIRA sections. We
> can call this page GIT and JIRA Process.
>
> Also, I could not find any place where we mention that contributors may
> create a GitHub pull request. Since we accept pull requests, that section
> definitely needs to be added.
>
> Thoughts?
>
>
>>
>> D.
>>
>> On Tue, Sep 29, 2015 at 12:41 AM, Konstantin Boudnik <cos@apache.org>
>> wrote:
>>
>>> Hmm...
>>>
>>> Negligence, n. : the trait of neglecting responsibilities and lacking
>>> concern
>>> syn : omission, oversight
>>>
>>> Doesn't sound catastrophic in my vocabulary, really. Does this
>>> > case of negligence and needs to be addressed accordingly.
>>> translate to "should face a firing squad without a trial of his peers"?
>>> Have I anywhere pointed a finger at you or anyone else? Or attacked
>>> someone? Why are you all upset and defensive about it?
>>>
>>> Cos
>>>
>>> On September 28, 2015 7:39:51 AM PDT, Raul Kripalani <raulk@apache.org>
>>> wrote:
>>> >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 <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