Code review comment for lp:~abentley/launchpad/bulk-insert

Revision history for this message
Richard Harding (rharding) wrote :

Overall this looks good. As someone not familiar with the code in question a few comments would have helped. It took me a lot of reads through newFromBazaarRevisions to trace what I think is going on in there. I look forward to a day when we can use namedtuples!

Is there a general frowning on *args? A lot of places needed to be updated to change their calling arguments to be wrapped in a list:
    RevisionSet().newFromBazaarRevisions([bzr_revision])

I can't help but wonder if allowing these to come in as
    def newFromBazaarRevisions(*revisions):

would end up being a bit cleaner. I've not seen much of that practice so far though.

review: Approve (code*)

« Back to merge proposal