[Mono-dev] revert of r104029/30/31

[Sebastien Pouliot]

Gert,

On Mon, 2008-05-26 at 11:10 +0200, Gert Driesen wrote:
> --------------------------------------------------
> From: "Sebastien Pouliot" <sebastien at ximian.com>
> Sent: Monday, May 26, 2008 1:21 AM
> To: <gert.driesen at pandora.be>
> Cc: "mono-devel-list" <mono-devel-list at lists.ximian.com>
> Subject: [Mono-dev] revert of r104029/30/31
> 
> > Gert,
> >
> > Please *post* your patches for review *before* committing them!
> > Those changes were obviously untested since they broke the unit tests.
> 
> Sorry about that. I was too confident that these changes were harmless, my 
> mistake.
> 
> I'll look into this, and - if you want - post an updated patch for review.

I'm not interested in reviewing a HUGE patch that mix the functional and
non-functional changes together. It's huge, hard to read, easy to
miss(*) and I already have spent too much time on this one (trying to
post-commit review it, reverting it, writing about it...)

(*) the fact that unit test caught one error does not mean there are no
others - and I don't believe the changes are worth regression(s).

> > They also mix formatting changes (not all in respect of mono style) with
> > code changes (parameter names) making this harder, than it has to be, to
> > review.
> 
> I'm pretty sure all the formatting changes respect the "mono style", but 
> feel free to correct me.
> 
> I know we had a "discussion" on whether braces for the class/namespace 
> should go on a separate line or not, and I know you thought they should go 
> on the same line. However, both the mono guidelines 

Please show me where in
http://www.mono-project.com/Coding_Guidelines
since all examples keep them on the same line ? (and this is not a code
block either ;-)

So it's one of the case that's unclear ? just like some other stuff that
was changed (or removed) since the original style guide ? 

I don't mind if, in doubt, you follow your own interpretation. However
please DO NOT start modifying existing code based on them (own
interpretation).

Fact is that the *style* is there to help everyone, just like doing
*small* patches are. Yes some old SD code breaks a *lot* of rules and I
slowly fixed some when the code needed fixing. 

Why ? because breaking the "as small as possible commits" rule makes
merging harder and SVN history less useful. Updating the current code
style is not gonna offset the cost of this (even less if it requires a
full review because it includes other changes as well).

> and several committers 
> on #mono confirmed that they should go on a separate line.

"confirmed" ? how ? who ? Maybe they just tell you what they do, or what
they like ?

Seriously we have a document for this. It's not perfect (and feel free
to start _another_ discussion about this) and there are a lot of rules I
didn't like (but I learned to follow them).

But like I said in doubt just *dont* touch it.

> I don't think I want to spend an hour or two splitting up the argument name 
> changes with the formatting changes, 

I understand this because I don't want to spend an hour or two to find
other problems in the diff (and it's clear it's something that needs to
be done).

> so if you prefer to keep the sources in 
> the state they were before my changes then I'll just revert my local 
> changes.
> 
> My formatting changes were limited to:
> 
> * changes loads of spaces to tabs
> * removing loads of trailing tabs
> * moving braces on class/namespace declaration to separate line (probably 
> less than 2% of my changes)

Even if 100% of the changes are ok and were made without other changes
(parameters) I still think (and would have told you) that it was not
worth the time you invested in this.

> > Also note that such *big* changes always cause a lot of conflicts for
> > the code maintainer(s), since they often have uncommitted code or patch
> > in testing. So a bit of warning before committing is a *minimum*.
> 
> True.

Yes, a lot of time could have been saved with a quick email (or ping on
IRC).

Sebastien