[Mono-dev] Contributing Code for ObservableCollection

[Alan McGovern]

Ok, so here's my comments (so far ;) ):

1) Xml comments are a no-go in the source files. If you want to commit
documentation (which is always appreciated!) you should add it to monodoc
and submit it as a separate patch. It should be possible to import your XML
comments into monodoc automatically so you don't have to re-type them. Once
they've been imported to monodoc, you'll have to strip them from the source
files.

2) You've duplicated some of the helper methods in your tests
(ObservableCollectionTest/NotifyCollectionChangedEventArgsTest). You could
create a base class which implements this helper methods and just have your
two test fixtures inherit from this, or you could define the methods
publicly in some static class and reference them from there. Less
duplication == good.

3)
+            public void Enter()
+            {
+                count++;
+            }
+
+            public void Dispose()
+            {
+                count--;
+            }

In Reentrant, are you sure this is the right behaviour? Can you make a
testcase where you call BlockReentrancy twice and then double dispose the
object given from the first call. Also, it might be worth writing a test to
ensure the exact same object is returned every time.

Other than that, it looks good to commit. When you've completed those
changes, send the updated patch here and I'll get to work on committing it.

Thanks for the great work,
Alan.

On Tue, Nov 4, 2008 at 1:39 PM, Brian O'Keefe <zer0keefie at gmail.com> wrote:

> Hi,
>
> There are indeed 3 test case files; I forgot to svn add them.  I made a new
> patch file with all of the files this time.  I'll attach the new one.
>
> ~Brian
>
>
> On Tue, Nov 4, 2008 at 4:25 AM, Alan McGovern <alan.mcgovern at gmail.com>wrote:
>
>> Hey,
>>
>> Maybe i'm just confused, but in your original email you said there were 3
>> testcase files, but I'm only seeing 1 modified testcase file.
>>
>> Is this right, or did you forget to svn add the new files before you
>> submitted the patch?
>>
>> Thanks,
>> Alan.
>>
>>
>> On Mon, Nov 3, 2008 at 11:36 PM, Brian O'Keefe <zer0keefie at gmail.com>wrote:
>>
>>> Hello,
>>>
>>> I've updated my code to conform to the coding guidelines.  My tests pass
>>> under both mono and the MS framework.  I'll attach the patch file for my
>>> changes.
>>>
>>> ~Brian
>>>
>>>
>>> On Sat, Nov 1, 2008 at 6:59 PM, Alan McGovern <alan.mcgovern at gmail.com>wrote:
>>>
>>>> Hey,
>>>>
>>>> Read here: http://www.mono-project.com/Contributing and also here
>>>> http://www.mono-project.com/Coding_Guidelines.
>>>>
>>>> Though my word is most definitely *not* the final word in any
>>>> discussion, I will say that if the code isn't covered by NUnit tests, it
>>>> won't be committed ;) So just ensure that for every method you've
>>>> implemented, there are one or more tests for it. From your email, it looks
>>>> like you've already done this, which is great!
>>>>
>>>> Also, just to verify, do all the tests pass when run under both the MS
>>>> framework and mono?
>>>>
>>>> Thanks.
>>>> Alan.
>>>>
>>>> 2008/11/1 Brian O'Keefe <zer0keefie at gmail.com>
>>>>
>>>>> Hello,
>>>>>
>>>>> I've been following the Mono project for a while, and had some free
>>>>> time, so I figured I'd use some of that time to contribute.  I've used the
>>>>> ObservableCollection class frequently
>>>>> (WindowsBase>System.Collections.Specialized.ObservableCollection) and
>>>>> noticed that it wasn't implemented in Mono.  This seemed like a fairly
>>>>> simple class to implement, so I thought I'd start there.  I have some code
>>>>> for ObservableCollection and its backing infrastructure (the
>>>>> NotifyCollectionChanged* classes) and matching test cases.
>>>>>
>>>>> What would be the best way to contribute the changes I made?  There are
>>>>> six modified files and three test case files.
>>>>>
>>>>> ~Brian
>>>>>
>>>>> _______________________________________________
>>>>> Mono-devel-list mailing list
>>>>> Mono-devel-list at lists.ximian.com
>>>>> http://lists.ximian.com/mailman/listinfo/mono-devel-list
>>>>>
>>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.ximian.com/pipermail/mono-devel-list/attachments/20081104/b0a8f878/attachment.html