[Grok-dev] Re: a nice grok todo item for a beginner
Philipp von Weitershausen
philipp at weitershausen.de
Sat Mar 24 13:29:15 EDT 2007
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
>>> 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
>> * 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
http://worldcookery.com -- Professional Zope documentation and training
More information about the Grok-dev