On 05/08/2013 05:07 PM, deadalnix wrote:
> 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.

I know this is a problem, but seeing a lot of changed code which has a different style makes it even harder to assess a pull request. Though it also shows that you could do a better job at getting your code into an acceptable shape, i.e. close to the surrounding style.

On Wed, 2013-05-08 at 17:07 +0200, deadalnix wrote:
[…]
> 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.

The Go team have gone a stage further, not only will they not look at code that is not properly formatted, neither will Go compilers.

They do have gofmt though which applies the style — which has some fundamental faults, but you have to live with them or not use Go. This has had the effect of retraining everyone using Go to use the global style.

Python doesn't quite go this far, but there is PEP-8 which is Python as Guido wants to see it formatted. There is a tool to support you getting it right.

C used to have an informally accepted style (which was awful) and until about 1981 all C code had the same style.

I have to admit the thing I really like about Go is the insistence on using tab character for leading indent. Now that I have discovered how to make tab display as 2 spaces instead of 8, I really like the lack of discussion about how many leading spaces per indent level in the source code. I would really like Python to have followed this rule.

Of course then you can bikeshed about whether the { goes on the end of the line or on the next line.  Clearly the only sane place is the end of the line, but many people hate that. And hate leads to suffering.

The positive lesson from Go is that application of one, and only one, formatting style, backed up by a tool incorporated into the compiler, does lead to the avoidance of bikeshedding and to community peace — just not world peace.
-- 
Russel. ============================================================================= Dr Russel Winder      t: +44 20 7585 2200   voip: sip:russel.winder@ekiga.net 41 Buckmaster Road    m: +44 7770 465 077   xmpp: russel@winder.org.uk London SW11 1EN, UK   w: www.russel.org.uk  skype: russel_winder

On 05/09/2013 10:48 AM, Russel Winder wrote:
> I have to admit the thing I really like about Go is the insistence on using tab character for leading indent. Now that I have discovered how to make tab display as 2 spaces instead of 8, I really like the lack of discussion about how many leading spaces per indent level in the source code. I would really like Python to have followed this rule.

I admit to liking "tabs for indentation, spaces for alignment": http://www.emacswiki.org/SmartTabs

... though I actually use Vi rather than Emacs to get it.  So that's clearly at least two counts on which I deserve to die. :-)

> Of course then you can bikeshed about whether the { goes on the end of the line or on the next line.  Clearly the only sane place is the end of the line, but many people hate that. And hate leads to suffering.

Don't know quite why, but I wound up quite liking the rule that Linus Torvalds suggested, which I think comes from K&R -- separate-line { for the opening of a function, same-line { for if(), while() statements and so on.

> The positive lesson from Go is that application of one, and only one, formatting style, backed up by a tool incorporated into the compiler, does lead to the avoidance of bikeshedding and to community peace — just not world peace.

Does it not make debugging a pain sometimes?  I'm fairly sure there are times when commenting out bits of code for debug purposes, I've in the process left other parts with incorrect indentation etc.

On Thursday, 9 May 2013 at 10:19:16 UTC, Joseph Rushton Wakeling wrote:
>
> Don't know quite why, but I wound up quite liking the rule that Linus Torvalds
> suggested, which I think comes from K&R -- separate-line { for the opening of a
> function, same-line { for if(), while() statements and so on.

Here's a breakdown of some of the more popular formatting styles: http://en.wikipedia.org/wiki/Indent_style.  I think D tends towards Allman style (which I think the astyle formatter calls BSD style).  My code is formatted a bit differently in terms of spacing around parens but I'd be happy to have it changed--I use Allman style these days too.

On Thursday, 9 May 2013 at 17:02:29 UTC, Sean Kelly wrote:
> On Thursday, 9 May 2013 at 10:19:16 UTC, Joseph Rushton Wakeling wrote:
>>
>> Don't know quite why, but I wound up quite liking the rule that Linus Torvalds
>> suggested, which I think comes from K&R -- separate-line { for the opening of a
>> function, same-line { for if(), while() statements and so on.
>
> Here's a breakdown of some of the more popular formatting styles: http://en.wikipedia.org/wiki/Indent_style.  I think D tends towards Allman style (which I think the astyle formatter calls BSD style).  My code is formatted a bit differently in terms of spacing around parens but I'd be happy to have it changed--I use Allman style these days too.

Actual style mostly don't matter. Consistency matter.

On Thursday, 9 May 2013 at 17:22:29 UTC, deadalnix wrote:
> On Thursday, 9 May 2013 at 17:02:29 UTC, Sean Kelly wrote:
>> On Thursday, 9 May 2013 at 10:19:16 UTC, Joseph Rushton Wakeling wrote:
>>>
>>> Don't know quite why, but I wound up quite liking the rule that Linus Torvalds
>>> suggested, which I think comes from K&R -- separate-line { for the opening of a
>>> function, same-line { for if(), while() statements and so on.
>>
>> Here's a breakdown of some of the more popular formatting styles: http://en.wikipedia.org/wiki/Indent_style.  I think D tends towards Allman style (which I think the astyle formatter calls BSD style).  My code is formatted a bit differently in terms of spacing around parens but I'd be happy to have it changed--I use Allman style these days too.
>
> Actual style mostly don't matter. Consistency matter.

Yep.  But it's easier to get consistency if everything follows the same style.  Commits to my code by other people, for example, typically follow Allman style despite it not being the way existing code is formatted in the module.

On Thursday, 9 May 2013 at 17:02:29 UTC, Sean Kelly wrote:
> Here's a breakdown of some of the more popular formatting styles: http://en.wikipedia.org/wiki/Indent_style.  I think D tends towards Allman style (which I think the astyle formatter calls BSD style).  My code is formatted a bit differently in terms of spacing around parens but I'd be happy to have it changed--I use Allman style these days too.

That's one property of Python's syntax that I like. You never argue about brace style because there are no braces.

Regarding the OP's post, I think this process isn't exclusive to just D's pull requests. I once worked in a web development position and experienced the same basic process of nitpicking. It tended to happen less after I figured out how to adopt their style. You eventually learn to tailor your code to the people you're presenting it to.

On Thu, May 09, 2013 at 07:22:28PM +0200, deadalnix wrote:
> On Thursday, 9 May 2013 at 17:02:29 UTC, Sean Kelly wrote:
> >On Thursday, 9 May 2013 at 10:19:16 UTC, Joseph Rushton Wakeling wrote:
> >>
> >>Don't know quite why, but I wound up quite liking the rule that Linus Torvalds suggested, which I think comes from K&R -- separate-line { for the opening of a function, same-line { for if(), while() statements and so on.
> >
> >Here's a breakdown of some of the more popular formatting styles: http://en.wikipedia.org/wiki/Indent_style.  I think D tends towards Allman style (which I think the astyle formatter calls BSD style). My code is formatted a bit differently in terms of spacing around parens but I'd be happy to have it changed--I use Allman style these days too.
> 
> Actual style mostly don't matter. Consistency matter.

+1. One of the things I learned in my current day job is to follow whatever style is in effect in the source you're modifying. As Andrei said in TDPL, even if it's not your personal preference, you won't turn into a goblin just for following whatever style is already there.

I've seen too many code changes that *don't* follow the prevalent style in a particular source: they are a perennial source of bugs and other problems, because it's very jarring to read a source that suddenly switches style for 5-6 lines or so, and then switches back. It distracts one from following the flow of the code, leading to oversights and hidden unnoticed bugs. Worse yet, people sometimes make changes with different default editor settings on tab stops, so what looks OK to them shows up as very jarring wrong indentations -- this is why I've come to understand why tabs are Teh Evil (nobody ever agrees on what they should be, and everyone contributes code using different tab stop settings that causes a gigantic mess in any project with non-trivial numbers of contributors). But the absolute worst is when everybody just follows their own style when editing source code -- the result is a chimeraic monstrosity where the coding style changes every 5 lines, and nobody can understand what the code does (and nobody dares fix the formatting 'cos then their name would show up in `svn blame`, and nobody wants to take responsibility for those malformatted lines of code that in all likelihood are crawling with bugs).


T

-- 
Skill without imagination is craftsmanship and gives us many useful objects such as wickerwork picnic baskets.  Imagination without skill gives us modern art. -- Tom Stoppard

On Wed, 08 May 2013 10:56:28 -0400, Benjamin Thaut <code@benjamin-thaut.de> 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.
>

I first of all want to say, good for you to try helping the D community.  This is something we need a lot of.  I also want to say that I am a contributor to both Phobos and Druntime, and I also have been largely unable to look at pull requests, shame on me.  This is super-frustrating for authors, and it's going to kill contributions if we don't fix it.

But I want to give a little bit more of an explanation here.  I was not involved in the pull request.  But I can see the history.  Some observations I have:

1. nitpicking of style is EASY, it requires no thought or real time, except to type the nit.  This makes it something more likely to occur before people have put any real thought into a pull request.  The lesson here should be to use the correct style so you don't have this problem.  I'm aware that we don't have a good definition, or good consistency, and we need to do both of those.
2. The people who complained about style/formatting were NOT the same people who questioned the utility of the pull request.  This is HUGELY important in understanding how this all went down.

For point 2, realize that people have their own schedules and own day-time jobs.  I literally ignored the newsgroups for 6 months because I did not have time to look at D at all.  From the point of view of the requester, it is really important to understand that there is no real order to pull requests to review.  It's not like people can't review more than one pull request, have to review them in a specific order, or have to review them at all.  And the goals and motivations for review can be different.  We don't all speak as one voice, so try to remember that.

This is something I think can be better managed with a collaboration tool (like trello), or at least a status notification of who is assigned, who is reviewing, what status is it in, etc.  There is a large nebulous thing called a review process that is right now defined loosely (AFAIK, the only requirement is we have 2 people review each request), and pretty much differs for every person.  This needs to change.  We need an official review process, and a well-defined order of how things should be done.  Otherwise, the requester has no clue what is happening, and even other reviewers have no idea what is happening.  I will start a new thread on this.

Sorry to read about this experience.  I hope we can all do better.

-Steve

On 5/9/2013 1:48 AM, Russel Winder wrote:
> The positive lesson from Go is that application of one, and only one,
> formatting style, backed up by a tool incorporated into the compiler,
> does lead to the avoidance of bikeshedding and to community peace — just
> not world peace.

I'd support the development of a dfmt, and using it to automatically reject pull requests for phobos that don't pass it.