Code review comment for lp:~leonardr/lazr.restful/multiversion-rename-params

Revision history for this message
Gary Poster (gary) wrote :

This looks good. I like the simplification.

On IRC I asked how you were going to use this data structure. You said that you would collect the data, then build the top-most version, pop the dict, build the next lower version, pop the dict, and so on. Therefore, this data structure will be very transient, and you don't need an API to access lower versions while the upper versions are still present. That's fine.

At this point, the whole thing could really be accomplished without a new class. Just the ``stack`` list of (name, dict) tuples, and direct calls to deepcopy for a new level in the stack would seem to add very little additional weight in the call sites. I'm tempted to suggest that you go that way, since it would remove code that might have little win, but on balance I'm fine with you seeing how this data structure will work in practice; maybe this class will actually be significantly nicer to work with for your use cases.

For the name, ``VersionedDict`` makes sense to me, particularly with new approach. OTOH, ``BleedThroughDict`` has the vampire-ish connotations that are so in vogue.

Gary

review: Approve

« Back to merge proposal