Code review comment for lp:~wgrant/launchpad/refactor-slavestatus

Revision history for this message
William Grant (wgrant) wrote :

> William thanks for the fix. The logic of what you have done looks good.
>
> However, I don't like the idiom you've employed of mutating the status
> dictionary inside the slaveStatus methods and then returning it. If you
> didn't want to mutate the original dictionary then making a copy, changing it,
> and returning that copy would make sense. Otherwise just modifying the status
> dictionary in place and not returning it is a reasonable thing to do (and
> preferred barring no solid reason to do the former). But to mutate and then
> return would lead one to think the dictionary was not modified in place, which
> we know it was.

Thanks for the review, Brad. I was a little concerned about that too, but wasn't
entirely sure of the cleanest solution. I've adopted your suggestion, and also
renamed the method to updateSlaveStatus to more accurately reflect its purpose.

If you approve, please land this once PQM opens.

« Back to merge proposal