surprising pattern of passing around and deepcopying a dict

Bug #1885712 reported by John Lenton
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
charm-k8s-mattermost
Fix Released
Low
Paul Collins

Bug Description

        pod_spec = self._make_pod_spec()
        pod_spec = self._update_pod_spec_for_canonical_defaults(pod_spec)
        pod_spec = self._update_pod_spec_for_clustering(pod_spec)
        pod_spec = self._update_pod_spec_for_k8s_ingress(pod_spec)
        pod_spec = self._update_pod_spec_for_licence(pod_spec)
        pod_spec = self._update_pod_spec_for_performance_monitoring(pod_spec)
        pod_spec = self._update_pod_spec_for_push(pod_spec)
        pod_spec = self._update_pod_spec_for_sso(pod_spec)
        pod_spec = self._update_pod_spec_for_smtp(pod_spec)

each of those update_pod calls makes a deepcopy throwing away the original,

        pod_spec = copy.deepcopy(pod_spec)

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_pod_spec()
        pod_spec._update_pod_spec_for_canonical_defaults()
        [ ... ]
        pod_spec._update_pod_spec_for_smtp()

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_pod_spec()
        self._update_pod_spec_for_canonical_defaults(pod_spec)

with that function modifying the dict in-place.

HTH,

Related branches

Revision history for this message
Paul Collins (pjdc) wrote :

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.

Revision history for this message
Paul Collins (pjdc) wrote :

Linked MP switches to in-place updates. It's probably better not to be surprising, and I'll bet debugging a missing "return pod_spec" would be frustrating.

Changed in charm-k8s-mattermost:
status: New → In Progress
importance: Undecided → Low
assignee: nobody → Paul Collins (pjdc)
Tom Haddon (mthaddon)
Changed in charm-k8s-mattermost:
status: In Progress → Fix Committed
Paul Collins (pjdc)
Changed in charm-k8s-mattermost:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.