python-oops-tools

Merge lp://staging/~beuno/python-oops-tools/show-newest-oopses into lp://staging/python-oops-tools

  1. show-newest-oopses
  2. Merge into trunk
Proposed by Martin Albisetti
Status: Needs review
Proposed branch: lp://staging/~beuno/python-oops-tools/show-newest-oopses
Merge into: lp://staging/python-oops-tools
Diff against target: 309 lines (+85/-18)
9 files modified
src/oopstools/oops/dboopsloader.py (+4/-4)
src/oopstools/oops/dbsummaries.py (+5/-2)
src/oopstools/oops/helpers.py (+6/-4)
src/oopstools/oops/oopsstore.py (+15/-4)
src/oopstools/oops/templates/newest.html (+12/-0)
src/oopstools/oops/test/oopsstore.txt (+32/-0)
src/oopstools/oops/test/test_runner.py (+2/-1)
src/oopstools/oops/views.py (+7/-2)
src/oopstools/urls.py (+2/-1)
To merge this branch: bzr merge lp://staging/~beuno/python-oops-tools/show-newest-oopses
Reviewer Review Type Date Requested Status
Benji York (community) code Approve
Review via email: mp+129752@code.staging.launchpad.net

Commit message

Add a view to see the newest oopses.

Description of the change

Add an option to see the newest oopses by project.

To post a comment you must log in.
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)
Reply
lp://staging/~beuno/python-oops-tools/show-newest-oopses updated
52. By Martin Albisetti

Review comments

Revision history for this message
Martin Albisetti (beuno) 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.

Done.

> 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.

It is. In this case, the worst that can happen is linking to something that isn't an oops.

> 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.

Done!

Reply
lp://staging/~beuno/python-oops-tools/show-newest-oopses updated
53. By Martin Albisetti

Start talking to the DB instead

Unmerged revisions

53. By Martin Albisetti

Start talking to the DB instead

52. By Martin Albisetti

Review comments

51. By Martin Albisetti

Add test

50. By Martin Albisetti

Clean up things a bit

49. By Martin Albisetti

Working, quick and dirty

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: