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

Atsushi Eno atsushi at ximian.com
Mon May 26 16:49:34 EDT 2008 

Yes, of course including all those arguments, I said "significant".

Atsushi Eno

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

More information about the Mono-devel-list mailing list