Code review comment for lp:~jelmer/launchpad/bug471148

Revision history for this message
Paul Hummer (rockstar) wrote :

<rockstar> jelmer, is this diff up to date? Does it have a dependent branch?
<jelmer> rockstar: the bug471148 one? That's current now
<rockstar> jelmer, okay.
<jelmer> rockstar: It's a fair bit smaller now that Muharems prerequisite branch has landed.
<rockstar> jelmer, you should set the prerequisite branch on the mp anyway, because it's informational.
<jelmer> rockstar: Even if that branch has already landed?
<rockstar> jelmer, yeah, it's still a good idea to say "I based my work off of this work."
<rockstar> Not a necessity, but for history's sake.
<jelmer> rockstar: Ah, that makes sense. I'll keep that in mind for next time.
<rockstar> jelmer, your tests need comments/documentation
<rockstar> lib/lp/soyuz/tests/test_processor.py and lib/lp/soyuz/tests/test_publishing.py
<rockstar> Actually, it looks like some of the tests have docstrings, so you'll probably just need to add the docstrings to the ones that lack them.
<jelmer> ok
<rockstar> jelmer, good on you for adding factory methods for your new work.
<jelmer> rockstar: Thanks
<rockstar> jelmer, other than the missing docstrings, this looks like very good work.

review: Approve (code)

« Back to merge proposal