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

[Gert Driesen]

Hey Sebastien,

For me, API compatibility with MS is important and as such I want to reduce 
the number of issues reported by our class status pages to zero (or as close 
as possible). Having several hundred issues reported because of mismatches 
in argument names can cause us to miss more important mismatches. I hope 
that makes it clear as to you why I want to get these argument name changes 
in.

One mistake I made is that - while fixing the argument names - I noticed a 
misture of spaces and tabs for indentation (and loads of trailing tabs), and 
decided to fix this. We all agree now that I could've split these changes. 
No problem.

A second mistake I made is that I forgot to run the unit tests after my 
changes. I don't think you have to convince me of the importance of unit 
tests, so we again agree on this.

Proposal:

If I have some time and you can make time to review it, I'll submit a patch 
for the argument name changes for review.

Agree?

Gert

--------------------------------------------------
From: "Sebastien Pouliot" <sebastien at ximian.com>
Sent: Monday, May 26, 2008 2:40 PM
To: "Gert Driesen" <gert.driesen at telenet.be>
Cc: "mono-devel-list" <mono-devel-list at lists.ximian.com>
Subject: Re: [Mono-dev] revert of r104029/30/31

> 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
>
> _______________________________________________
> Mono-devel-list mailing list
> Mono-devel-list at lists.ximian.com
> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>