Code review comment for lp:~cr3/launchpad/hwsubmissionset_search

Revision history for this message
Robert Collins (lifeless) wrote :

Code wise - a few thoughts.

The function signature you've added confuses me.
What does this mean:
+ :param submitted_before: Limit results to submissions submitted
78 + before this date inclusively.
79 + :param submitted_after: Limit results to submissions submitted
80 + after this date exclusively.

The inclusive and exclusive bits could refer to being a half-open or closed range endpoint, or to the way that the constraint is connected to the rest of the search.

Perhaps a better thing to say is 'Exclude results submitted after this date'; 'Exclude results submitted before or on this date'

There is one little nit on style, which you can ignore if you like (though the next person through will likely tidy it up: vertical whitespace in a function is frowned upon in PEP8: '
    Use blank lines in functions, sparingly, to indicate logical sections.
'

The fact you're extending a doctest is killing baby kittens in alaska. Did you consider unit testing this?

Stub, we could make this a -1 patch can't we? That way it can all land on devel and we can do the indices live? hwsubmissions are our current #1 timeout...

review: Approve

« Back to merge proposal