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

Mon May 26 12:39:31 EDT 2008

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