[Grok-dev] Re: a nice grok todo item for a beginner

[Philipp von Weitershausen]

Darryl Cousins wrote:
> On Mon, 2007-03-19 at 18:48 -0600, Philipp von Weitershausen wrote:
>> Darryl Cousins wrote:
>>> On Mon, 2007-03-19 at 23:17 +0100, Martijn Faassen wrote:
>>>> Hi there,
>>>>
>>>> There's an open todo item that would be nice for a beginner to do: 
>>>> change the admin page of grok to allow users to delete/uninstall grok 
>>>> applications.
>>> Checkboxes and delete form added. Simple Delete view deletes the
>>> selected items.
>> Thank you for this! I have a few comments, though:
>>
>> * Please make sure your checkins appear on the checkins at zope.org list. 
>> It's incomprehensible for the rest of us to follow your patches 
>> otherwise. Bug Jim Fulton about this, it's not an automatic process 
>> unfortunately.
>>
>> * In the Delete.render() method you write::
>>
>>          if not type(items) == type([]):
>>              ...
>>
>>    The preferred spelling would be::
>>
>>          if not isinstance(items, list)
>>              ...
>>
>> * You left a 'print' statement in the code. I think that needs to go.
> 
> Thanks for the comments. I've actioned and committed the above.

I have some more comments:

* When you implemented this feature, you didn't run Grok's tests. 
Therefore you didn't notice that you actually broke a functional test in 
Grok. I fixed this for you now. In the future, always (ALWAYS, no 
excuses) run the tests before checking in. Even if you're just fixing up 
whitespace or getting rid of an unused import, it's always good to know 
that your "fix" doesn't break something else.

* Furthermore, you didn't WRITE tests for the new functionality. We 
require tests (unit tests and/or functional tests) for every major 
change (feature, bugfix, etc.). In this case you should write a 
functional test that exercises the delete functionality. I did not write 
that for you. Please do it soon.

> With the checkins list. I got a mail from checkins-bounces because I
> wasn't subscribed to the list. I subscribed immediately after the first
> commit, so it should be ok now. If not, let me know and I'll see what I
> need to do (ask Jim probably).

The last point above stresses the importance of the commit list. If we 
see the diff for every commit, we can review it much much more easily 
and then call on the "checkin police" if necessary.

Btw, I still don't see Tim's checkins appearing. Martijn has been 
digging around his branch and wonders if tests are there or not... If 
we'd be able to see the commit diffs, that question could be answered 
quite easily.


-- 
http://worldcookery.com -- Professional Zope documentation and training