www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - D pullrequest review process rant

reply Benjamin Thaut <code benjamin-thaut.de> writes:
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
May 08 2013
next sibling parent "Dicebot" <m.strashun gmail.com> writes:
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.
May 08 2013
prev sibling next sibling parent reply "deadalnix" <deadalnix gmail.com> writes:
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.
May 08 2013
next sibling parent Benjamin Thaut <code benjamin-thaut.de> writes:
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
May 08 2013
prev sibling next sibling parent Martin Nowak <code dawg.eu> writes:
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.
May 08 2013
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
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.
May 09 2013
parent Walter Bright <newshound2 digitalmars.com> writes:
On 5/9/2013 11:15 AM, Brad Anderson wrote:
 On Thursday, 9 May 2013 at 18:10:47 UTC, Walter Bright wrote:
 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.

I think even neater would be a tool that sends a pull request to the source repo/branch with the format changes so the author can just apply them with a click.

It could also start off as something as simple as: 1. no tabs 2. LF line endings (no CRLF) 3. no trailing whitespace
May 09 2013
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
On 2013-05-09 19:02, 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.

DMD uses the weird Horstmann style in some places. Which in my opinion is horrible. -- /Jacob Carlborg
May 10 2013
parent "Daniel Murphy" <yebblies nospamgmail.com> writes:
"Jacob Carlborg" <doob me.com> wrote in message 
news:kmii5f$1nut$1 digitalmars.com...
 DMD uses the weird Horstmann style in some places. Which in my opinion is 
 horrible.

This is the old code and is discouraged in new code.
May 10 2013
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
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.
May 08 2013
prev sibling next sibling parent Timothee Cour <thelastmammoth gmail.com> writes:
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

May 08 2013
prev sibling next sibling parent "Sean Kelly" <sean invisibleduck.org> writes:
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.
May 08 2013
prev sibling next sibling parent Nick Sabalausky <SeeWebsiteToContactMe semitwist.com> writes:
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


May 08 2013
prev sibling next sibling parent "Jonathan M Davis" <jmdavisProg gmx.com> writes:
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
May 08 2013
prev sibling next sibling parent Timothee Cour <thelastmammoth gmail.com> writes:
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.
May 08 2013
prev sibling next sibling parent Russel Winder <russel winder.org.uk> writes:
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, 2013-05-08 at 17:07 +0200, deadalnix wrote:
[=E2=80=A6]
 When you read a lot of code, if the presentation is correct, it=20
 disappear and the content become apparent. Otherwise, it is much=20
 more work to get to the content.
=20
 Many project simply reject code that is not formatted properly.=20
 Not kidding !
=20
 The solution is o provide a code formatter here, not to change=20
 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 =E2=80=94 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 =E2=80=94= just not world peace. =20 --=20 Russel. =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D Dr Russel Winder t: +44 20 7585 2200 voip: sip:russel.winder ekiga.n= et 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
May 09 2013
prev sibling next sibling parent Joseph Rushton Wakeling <joseph.wakeling webdrake.net> writes:
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.
May 09 2013
prev sibling next sibling parent "Sean Kelly" <sean invisibleduck.org> writes:
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.
May 09 2013
prev sibling next sibling parent "deadalnix" <deadalnix gmail.com> writes:
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.
May 09 2013
prev sibling next sibling parent "Sean Kelly" <sean invisibleduck.org> writes:
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.
May 09 2013
prev sibling next sibling parent "w0rp" <devw0rp gmail.com> writes:
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.
May 09 2013
prev sibling next sibling parent "H. S. Teoh" <hsteoh quickfur.ath.cx> writes:
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
May 09 2013
prev sibling next sibling parent "Steven Schveighoffer" <schveiguy yahoo.com> writes:
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
May 09 2013
prev sibling next sibling parent "Brad Anderson" <eco gnuk.net> writes:
On Thursday, 9 May 2013 at 18:10:47 UTC, Walter Bright wrote:
 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.

I think even neater would be a tool that sends a pull request to the source repo/branch with the format changes so the author can just apply them with a click.
May 09 2013
prev sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 5/8/2013 7:56 AM, 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.

For me, the fundamental issue with it was the design - use tagged variants or virtual functions? I agree with you that without a decision on that, proceeding with an implementation is not worth the effort. BTW, one issue is nearing resolution. D will soon provide a dll for phobos for Linux, and it is intended to migrate this to all platforms, which will resolve the ODR problem.
May 09 2013
next sibling parent Rainer Schuetze <r.sagitario gmx.de> writes:
On 09.05.2013 20:15, Walter Bright wrote:
 BTW, one issue is nearing resolution. D will soon provide a dll for
 phobos for Linux, and it is intended to migrate this to all platforms,
 which will resolve the ODR problem.

A shared phobos library will not solve the ODR problem, because TypeInfo for instances of class or struct templates will still be generated into every binary that instantiates them if they do not happen to be instantiated by phobos itself. I'm undecided whether these could be treated as different classes/structs even if they have the same name. Also considering possible different memory layout due to different compile options: welcome to DLL hell.
May 09 2013
prev sibling parent Benjamin Thaut <code benjamin-thaut.de> writes:
Am 09.05.2013 20:15, schrieb Walter Bright:
 On 5/8/2013 7:56 AM, 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.

For me, the fundamental issue with it was the design - use tagged variants or virtual functions? I agree with you that without a decision on that, proceeding with an implementation is not worth the effort. BTW, one issue is nearing resolution. D will soon provide a dll for phobos for Linux, and it is intended to migrate this to all platforms, which will resolve the ODR problem.

Well that is nice for you, but for my use cases this will most likely not work, and thus I'm just going to continue maintaining my own version of druntime. For example I'm reloading classes at runtime which happens via a dll. As a result the TypeInfo of that class will exist twice (if you reload once) because the dll with the new class binary will contain the new TypeInfo object and thus violating the ODR. For a in depth explanation see: http://3d.benjamin-thaut.de/?p=25 Kind Regards Benjamin Thaut
May 09 2013