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 reply 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
parent reply 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
parent reply 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
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 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 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 Russel Winder <russel winder.org.uk> writes:
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
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 reply "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
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 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
parent reply "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
next sibling parent reply "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
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 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 "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 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 "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 "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 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