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

Atsushi Eno atsushi at ximian.com
Mon May 26 13:04:04 EDT 2008 

Probably I was misunderstood.

What I've suggested here was to *disable* showing parameter name
difference by default, and show them only when someone ran
corcompare locally with some command line options (or whatever
configurable) to "fix" parameter names.

Atsushi Eno


Jonathan Pobst wrote:
> That's why its great when someone takes the time and contributes the 
> changes.  :)
> 
> There is still tons of corcompare changes needed, so lets get this 
> process figured out and not discourage anyone from tackling the issue.
> 
> Jonathan
> 
> 
> Atsushi Eno wrote:
>> IMO the parameter names are just significant, not important. I mean,
>> it's like "significant space" in XML document. 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".
>>
>> Personally I dislike that corcompare change. It could have been
>> optional to work only for compatibility enthusiasts.
>>
>> 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
>>
>>
> 
>

More information about the Mono-devel-list mailing list