I recently got very disappointed by the review process of a pull request I did on druntime: https://github.com/D-Programming-Language/druntime/pull/370

This is how it went:

1) First everyone nitpicked about code formatting
2) I fixed all the nitpicks which was quite some work.
3) Pull request stalls for a month.
4) Even more nitpicks.
5) Even more work.
6) Reviewers actually start thinking about the problem behind the pull request.
7) Problem is not important enough, review request gets rejected.
8) All the work, including fixing nitpicks is wasted.

So what I'm trying to say is, that maybe a pull request should first be analyzed if it is actually worth putting more work into it before starting with the nitpicks. I don't know if the review process is already defined somewhere, if not it might be worth doing so.

-- 
Kind Regards
Benjamin Thaut

On Wednesday, 8 May 2013 at 14:56:23 UTC, Benjamin Thaut wrote:
> ...

It is not publicly defined anywhere, pretty much as anything else about D _development_ process. People just come and make comments about stuff they notice. At some point someone with merge rights come and either highlight crucial issue or merges. As far as I have followed pull requests, that is how it works. Often it takes months, lot of them.

On Wednesday, 8 May 2013 at 14:56:23 UTC, Benjamin Thaut wrote:
> I recently got very disappointed by the review process of a pull request I did on druntime: https://github.com/D-Programming-Language/druntime/pull/370
>
> This is how it went:
>
> 1) First everyone nitpicked about code formatting
> 2) I fixed all the nitpicks which was quite some work.
> 3) Pull request stalls for a month.
> 4) Even more nitpicks.
> 5) Even more work.
> 6) Reviewers actually start thinking about the problem behind the pull request.
> 7) Problem is not important enough, review request gets rejected.
> 8) All the work, including fixing nitpicks is wasted.
>
> So what I'm trying to say is, that maybe a pull request should first be analyzed if it is actually worth putting more work into it before starting with the nitpicks. I don't know if the review process is already defined somewhere, if not it might be worth doing so.

nit is important.

When you read a lot of code, if the presentation is correct, it disappear and the content become apparent. Otherwise, it is much more work to get to the content.

Many project simply reject code that is not formatted properly. Not kidding !

The solution is o provide a code formatter here, not to change the review process.

Am 08.05.2013 17:07, schrieb deadalnix:
> nit is important.
>
> When you read a lot of code, if the presentation is correct, it
> disappear and the content become apparent. Otherwise, it is much more
> work to get to the content.
>
> Many project simply reject code that is not formatted properly. Not
> kidding !
>
> The solution is o provide a code formatter here, not to change the
> review process.

I didn't say that nitpicking is not important, but it has to happen at the right time.

-- 
Kind Regards
Benjamin Thaut

On 05/08/2013 04:56 PM, Benjamin Thaut wrote:
> So what I'm trying to say is, that maybe a pull request should first be analyzed if it is actually worth putting more work into it before starting with the nitpicks. I don't know if the review process is already defined somewhere, if not it might be worth doing so.

I had a similar experience of code-style corrections arriving before deeper review.  My impression was that this was because once you've taught the style to a contributor once, you don't have to teach them again -- suffice to say that it made later work easier because I knew what to do in order to avoid ever getting those kind of nitpicks.  In other words we could concentrate _just_ on the problem and not on the style.

I'm sorry you had a bad experience, though, and I can see exactly why it would be offputting.

I agree with deadalnix, we need a dfmt tool to autoformat submissions.
Then this problem goes away.
uncrustify has support for D, we just need to write the options file
that everyone agrees upon, or, even better, a custom tool.
We could also automate the formatting part of the review process by
running a linter that checks that code==format(code) and automatically
rejects code that violates this. Saves a lot of time for both writer
and reviewer. These tools are used very effectively in some companies.


On Wed, May 8, 2013 at 8:10 AM, Benjamin Thaut <code@benjamin-thaut.de> wrote:
> Am 08.05.2013 17:07, schrieb deadalnix:
>
>> nit is important.
>>
>> When you read a lot of code, if the presentation is correct, it disappear and the content become apparent. Otherwise, it is much more work to get to the content.
>>
>> Many project simply reject code that is not formatted properly. Not kidding !
>>
>> The solution is o provide a code formatter here, not to change the review process.
>
>
> I didn't say that nitpicking is not important, but it has to happen at the right time.
>
>
> --
> Kind Regards
> Benjamin Thaut

On Wednesday, 8 May 2013 at 14:56:23 UTC, Benjamin Thaut wrote:
> I recently got very disappointed by the review process of a pull request I did on druntime: https://github.com/D-Programming-Language/druntime/pull/370
>
> This is how it went:
>
> 1) First everyone nitpicked about code formatting
> 2) I fixed all the nitpicks which was quite some work.
> 3) Pull request stalls for a month.
> 4) Even more nitpicks.
> 5) Even more work.
> 6) Reviewers actually start thinking about the problem behind the pull request.
> 7) Problem is not important enough, review request gets rejected.
> 8) All the work, including fixing nitpicks is wasted.

The timeliness of feedback is largely my fault.  I hadn't been spending much time on D last fall.  This is something I intend to change.  For large changes like this though, I do think the submitter needs to champion them if they want a timely review.  In most cases, if the change is an enhancement and it's as substantial as this, people will put off reviewing it simply because of the time required to do so.

Personally, I find that good readability sometimes dictates selective exceptions to a codebase's usual style guidelines. That is, I find that code formatting rules are best used as general guideline or rules-of-thumb, rather than strict hard and fast rules. Automated formatting enforcement would get in the way of that.

That said, I certainly understand the need for and benefits of minimizing manual overhead in the whole submissions process. So I'm not going to say that I'm opposed to the idea, merely putting out my $0.02 FWIW.


On Wed, 8 May 2013 10:13:15 -0700
Timothee Cour <thelastmammoth@gmail.com> wrote:

> I agree with deadalnix, we need a dfmt tool to autoformat submissions.
> Then this problem goes away.
> uncrustify has support for D, we just need to write the options file
> that everyone agrees upon, or, even better, a custom tool.
> We could also automate the formatting part of the review process by
> running a linter that checks that code==format(code) and automatically
> rejects code that violates this. Saves a lot of time for both writer
> and reviewer. These tools are used very effectively in some companies.
> 
> 
> On Wed, May 8, 2013 at 8:10 AM, Benjamin Thaut <code@benjamin-thaut.de> wrote:
> > Am 08.05.2013 17:07, schrieb deadalnix:
> >
> >> nit is important.
> >>
> >> When you read a lot of code, if the presentation is correct, it disappear and the content become apparent. Otherwise, it is much more work to get to the content.
> >>
> >> Many project simply reject code that is not formatted properly. Not kidding !
> >>
> >> The solution is o provide a code formatter here, not to change the review process.
> >
> >
> > I didn't say that nitpicking is not important, but it has to happen at the right time.
> >
> >
> > --
> > Kind Regards
> > Benjamin Thaut

On Wednesday, May 08, 2013 14:08:10 Nick Sabalausky wrote:
> Personally, I find that good readability sometimes dictates selective exceptions to a codebase's usual style guidelines. That is, I find that code formatting rules are best used as general guideline or rules-of-thumb, rather than strict hard and fast rules. Automated formatting enforcement would get in the way of that.

Exactly, code formatters inevitably end up mangling code or just plain formatting some of it badly, because sometimes you need to make exceptions to the formatting rules in order to format the code in an appropriately legible and maintainable way. They may help make things more consistent, but they just aren't flexible enough, and I very much hope that we never have to deal with one in Phobos. Basic stuff like where the braces go and spaces vs tabs might be okay, but I definitely don't want to see anything trying to automatically formatting code with regards to where spaces go or how many there are or where lines are broken up (which is a classic case where formatters tend to do badly) or anything like that.

- Jonathan M Davis

Can you point me to a few places in phobos where you worry the autoformatter will be undesirable ?

We use something like clang_format on a large code base and it just works.
For the rare cases where one wants to override the autoformatter, we
can include a special tag in comments.
The need for those annotations should be quite rare.
Example:

// @NOFORMAT_BEGIN
void my_specially_formatted_fun(){
  int[]x=[0,1,2,
            4,5,6];
}
// @NOFORMAT_END

or with a single statement, which shall apply to the following logical block:
// @NOFORMAT_BLOCK
void my_specially_formatted_fun(){
  int[]x=[0,1,2,
            4,5,6];
}
(Another option would be user defined attributes although I doubt
that's a good use of those).

Note, comments shouldn't be touched typically, but could be formatted
on demand with dfmt begin_line end_line (clang_format has such an
option).
As for line breaks, I would argue that a much better behavior is to
simply use soft line wraps, as supported by most modern editors, so
there would be no need for them (I personally hate hard line wraps
which creates unnecessary overflow/underflow problems in comments and
many other issues).

I personally don't care much for minute formatting details but some
do, so having such a tool makes formatting related problems just go
away.
I believe it can be done for D with almost no need for special
override markers, and the benefits will largely outweigh the
inconvenience of few places requiring them.