launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Review: Approve code

Hi Ian,

This new email header looks to be a big win.  Thanks for doing the branch.

I note that you filed the bug that this branch fixes and you don't mention any pre-implementation discussions.  It's comforting as a reviewer to see some hint of collaboration so that it is obvious the developer isn't doing something crazy on his own.  Bear that in mind for the future.

* s/can be made embargoed/can be embargoed

* It would be nice to make a helper function out of the complicated print statement that appears beginning at lines 111, 158 and 185 of your diff.

* Thanks for cleaning up the dead code.

* Have plans been made with the Product Team to announce the availability of this new header via a blog post?  I think our users would be keen to start using it if it is properly publicized.  Talk to Matthew or Dan if you haven't yet.

Thanks again!
-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-mail-information-type/+merge/103793
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.