surprising pattern of passing around and deepcopying a dict
Affects | Status | Importance | Assigned to | Milestone | |
---|---|---|---|---|---|
charm-k8s-mattermost |
Fix Released
|
Low
|
Paul Collins |
Bug Description
pod_spec = self._make_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
pod_spec = self._update_
each of those update_pod calls makes a deepcopy throwing away the original,
pod_spec = copy.deepcopy(
and it seems weird to be doing all this copying instead of just updating in-place. If pod_spec were a more ad-hoc class you'd do this as
pod_spec = self._make_
[ ... ]
Just because it's a dict instead of a more specialised object doesn't mean you need to do all the extra copying; particularly when the name of the function already implies 'modifying in place'. In other words, I'd expect this code to be more like
pod_spec = self._make_
with that function modifying the dict in-place.
HTH,
Related branches
- Tom Haddon: Approve
- Canonical IS Reviewers: Pending requested
-
Diff: 258 lines (+38/-51)2 files modifiedsrc/charm.py (+17/-42)
tests/unit/test_charm.py (+21/-9)
Changed in charm-k8s-mattermost: | |
status: | In Progress → Fix Committed |
Changed in charm-k8s-mattermost: | |
status: | Fix Committed → Fix Released |
The idea was to make it easier to write tests for these functions and also not accidentally update a dict in place that for some reason doesn't end up getting replaced by the caller. Possibly these are not worth all the extra boilerplate.
I thought of writing a decorator to handle this, but then you have code that looks like it's updating in place when it isn't.
Only one of these functions is being tested so far, so it wouldn't be too much work to change.