Code review comment for lp:~beuno/python-oops-tools/show-newest-oopses

Revision history for this message
Benji York (benji) wrote :

The whitespace fixes are appreciated. One class of whitespace change
looks like a search/replace error though: lines 8 and 103 of the diff
have one-element lists that included a trailing comma. That was
somewhat odd, but the added space after the comma is even oddder. I
suggest removing both the comma and the space.

I don't know much about the on-disk layout for OOPS storage, but I would
be a little concerned that non-oops files and directories would confuse
the "newest" method. If this trait is common to the rest of the
OOPS-tools code, then my concern can be ignored.

The test for the new "newest" function (line 239 of the diff) contains a
dependency on the order of elements returned from a call the "values"
method of a dict. That order can vary, so the returned list needs to be
sorted in the doctest in order to avoid spurious test failures.

The above applies to the test on line 246 of the diff as well.

Once these small issues are fixed, the code is good to land.

review: Approve (code)

« Back to merge proposal