Code review comment for lp:~vila/uci-engine/1302471-fake-rabbit

Revision history for this message
Vincent Ladeuil (vila) wrote :

> You have a merge conflict in this,

Yeah, that's weird, I resolved it locally but haven't pushed the result, sorry about that.

> and it also seems like you may be combining
> 3 different MPs, so I'm wondering if this was intentional?

Well, yes, all the changes are related to each other so splitting them means the reviewer has to jump around MPs to understand *why* they are done.

> I think there are
> 3-4 change-sets:
>
> 1) re-ordering of parameters in functions
> I'd separate this so this diff will be small and easy to ack.

I can do that but if the related MPs are landed at different times the resulting history will lose the reasoning leading to that change.

>
> 2) FakeTaskQueue
> I think this plus the change to the test-runner
>
> 3) tests/test_rabbit.py
>
> 4) amqp_utils split up of send into send/declare_queue
>
> I might be reading too fast, but I think all 4 of these changes are
> independent of one another (with the expception or #4).

« Back to merge proposal