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

Revision history for this message
Marc Tardif (cr3) wrote :

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

Done.

> Use blank lines in functions, sparingly, to indicate logical sections.

I typically consider each conditional statement as a logical section but
I can appreciate how this can result in lots of vertical noise, so I've
increased the signal.

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

I was torn between writing a new set of unit tests or extending the
existing code base which was relying on doctests. I agree the former
would've been preferable but would've been a much more significant
undertaking than the patch itself. So, if the hwdb is likely to get
more attention, then I think it'll be worthwhile to move the existing
doctests to unit tests and save a few kittens in the process.

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

I renamed the patch following stub's comment and made sure to update
LaunchpadDatabaseRevision accordingly. Finally, I just pinged someone
in #launchpad-dev to help with landing the patch.

Thanks for all the advice!

« Back to merge proposal