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

Mon May 26 13:17:48 EDT 2008

On Tue, 2008-05-27 at 01:14 +0900, Atsushi Eno wrote:
> IMO the parameter names are just significant, not important. I mean,
> it's like "significant space" in XML document. 

My understanding is that the parameters names are important in some
languages (VB?) since they can be used inside code. So I guess they can
be quite important for some folks...

> If they are regarded
> as important, I strongly suggest to not report them. They cost us
> lots of hours or days to fix them and hence practically block the
> actual API fixes that are actually "important".

        <note>I've must be doing too much C/C++ since I did not notice
        the status pages were updated for parameters</note>

> Personally I dislike that corcompare change. It could have been
> optional to work only for compatibility enthusiasts.

...and by "some folks" I mean that I won't fix them myself but, like
always, patches (not commits ;-) are most welcome :-)

Sebastien

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