On Mon, Sep 23, 2013 at 10:35:58AM +0200, David Henningsson wrote:
> On 09/21/2013 01:21 PM, poljar wrote:
> > On Thu, Sep 19, 2013 at 11:53:52PM +0200, David Henningsson wrote:
> >> On 09/19/2013 01:17 PM, Damir Jeli? wrote:
> >>> Sorry if I'm a little bit annoying here but I'd like to keep our from
> >>> now on with as little style inconsistency as possible.
>
> Let me just ask for clarification about the sentence above. When you
> submitted patches a while ago, I remember coding style patches changing
> things that were not in our coding style rules, e g, you changed every
> two newlines into one.
Sorry if I haven't been clear enough there. It should be:
Sorry if I'm a little bit annoying here but I'd like to keep our
tree from now on with as little style inconsistency with regard to our
rules as possible.
I didn't realize that this wasn't in the coding style. I've submitted
patches with two newlines in the past and Tanu [1] asked me to remove them
so I wrongfully thought that it was in our coding style. Sorry about
that.
>
> With that patch, you increased the coding style consistency. It sounds
> like you want to enforce those changes by adding more coding style
> rules; and that's what I'm turning against: I'd like fewer coding style
> rules, not more of them.
No. I don't want to increase or decrease the number of our rules here,
I just want to protect my time that I invested into fixing the inconsistencies.
If we do happen to decrease the number of rules I would be fine with
that as well.
>
> >>> I haven't done
> >>> any proper review/testing on this, just a quick coding style check.
> >>
> >> Sorry if I'm even more annoying here ;-) but perhaps you haven't heard
> >> my personal view on this matter.
> >>
> >> I totally disagree. I would instead like to keep our coding style rules
> >> to a bare minimum to keep the code decently readable. Anything else is
> >> just an additional road block towards what's important: spending as much
> >> time as possible fixing bugs and implementing new features, rather than
> >> spending time complying to coding style rules.
> >>
> >> So; for the opening bracket on newline - I think our current coding
> >> style is wrong and should be changed for functions, and I always forget
> >> that we have this odd style here. You're okay to comment on that,
> >> because it is in our coding style rules, but I would appreciate more, as
> >> you say, "any proper review/testing", rather than a simple coding style
> >> check. You focus on what looks good on the surface, but what really
> >> matters is real code quality - i e, whether the code is going to solve
> >> our users' problems without causing annoying bugs, or not.
> >>
> >
> > I understand that a full review is more useful, but a full review also
> > consists of a 'code style review' and I consider doing only one part of a
> > review still useful. On the Wayland mailing list everyone is encouraged to
> > do reviews [1] and doing only one part of a review means that the person that
> > comes along next doesn't need to do that part as well.
> >
> > I don't mean to say that I plan to do only coding style reviews on a
> > regular basis. I only did so this time because you relatively recently
> > introduced a style inconsistency [2] that slipped through a full review [3].
> >
> > Yes, I agree, if I have to choose between good style and new features or bug
> > fixes I would choose the latter, but this is a false dichotomy. We don't
> > need to choose here.
>
> Fair point. I just get the feeling that sometimes people do choose the
> "only coding style check" type of review just to feel they've done
> *something*, whereas the full review is what's really needed.
>
> Or people that can do a full review, might look at the patch and see
> that it has already been answered to, and think that maybe they should
> review something else instead.
>
I would argue that the PulseAudio community is to small for that to
happen, but on the other hand I already said that I don't plan to do
this regularly so you don't have to worry about that.
If there happen to be simpler patches and I feel qualified to review
them I'll try to make a full review. As I almost did with Aarun's patch [2].
> >>> If you're really lazy you can use my sed script to fix this issues (well
> >>> the opening bracket on newline issues at least).[2]
> >>
> >> For the stuff that's *not* in the coding style wiki page, I'm even
> >> lazier. If you prefer a different style than I happen to write, feel
> >> free to run your scripts every six months or so and submit a patch for
> >> it, and I won't complain if somebody else reviews and commit it. (Well,
> >> unless it severely decreases readability somehow.)
> >
> > I think you misunderstood me here. I don't demand that you to change anything
> > about the newlines here and no, I don't plan to be a cleaning lady for our
> > tree.
> >
> > Let me rephrase my original mail in a not so 'cold and professional' tone.
> >
> > The comment about the bracket issue:
> > 'Hey David, please don't forget our coding style.'
> >
> > The comment about the newlines:
> > 'This looks a little bit odd considering the rest of our tree but do as
> > you please, or if Tanu has a better idea do as he suggests'
> >
> > Again sorry if this has annoyed you or if I have worded my thoughts
> > poorly. I just wanted to make sure that my recent style fixes were not
> > thrown away effort.
>
> No worries, no offence taken, and I admit being a bit tired when I wrote
> back to you.
>
OK. Thats nice to hear. In the future I'll make sure to not annoy you
before the weekend, only after. :P
[1] http://lists.freedesktop.org/archives/pulseaudio-discuss/2012-May/013583.html
[2] http://lists.freedesktop.org/archives/pulseaudio-discuss/2013-August/018409.html
