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

Mon May 26 09:12:26 EDT 2008

Hello Gert,

On Mon, 2008-05-26 at 14:49 +0200, Gert Driesen wrote:
> 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.

I'm in *total* agreement about fixing parameters names (even if I have
yet to see a bug report about it) and all other issues pointed by the
class status page (if it's not important enough to fix then it should
not be displayed on those pages).

> 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.

/me nods

However the unit test failure was "pure luck" (well from my point of
view ;-). For that matter I did a quick review of the patches (from the
mailing-list archive) and I did not spot it (and I prefer blaming the
size of the patch that my eyesight ;-).

> 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?

Yes. Thanks!
Sebastien

> 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
> > 
> 