On Jan 28, 2008 6:20 PM, Giampaolo Rodola' <report@bugs.python.org> wrote:
>
> Giampaolo Rodola' added the comment:
>
> > Well, if you have an opinion, feel free to leave a comment in this
> > issue about it. I will most likely be the one who does the checkin and
> > I will read this issue before I commit.
>
>  * One of the things I dislike is the fact that the student used "self.g
> = gdbm.open(self.filename, 'c')" in setUp method since if that fails
> there will be a NameError exception raised in tearDown method and the
> errors reported by unittest would be 2 where in fact it would be only 1.
> Probably a thing like this one would have been better:
>
>     def setUp(self):
>         self.g = None
>
>     def tearDown(self):
>         if self.g is not None:
>             self.g.close()
>           ...
>
>     def test_it(self):
>         self.g = gdbm.open(self.filename, 'c')
>         ...
>

Could also still open perform the open() call in setUp() and do what
you need in the tearDown() to not be an error. There should probably
also be a way to not cause an error if the file path is just not
writable as that is not a sign of a failing test but an unavailable
resource.

> - Another thing I don't like is that os.unlink(self.filename) executed
> in tearDown would be better if protected by a try/except statement.
>

Even better is test.test_support.unlink() which handles those details for you.

> - +1 for the self.g.close() used by the student in the tearDown method.
> That was a thing I haven't considered and it's not included in my patch.
>
> - +0.5 for the "test_modes" method added by the student. Maybe it's
> useful, maybe it's not, I don't know.
>
>
> Aside from that I don't see other relevant differences since the code is
> really minimalistic. Feel free to commit the patch you consider to be
> the best.
> If you want me to do so I can provide a merged version of my patch
> including the differences I described above.

Fine by me. Or you can wait until I have time to look at your code.
It's up to you.