dash-docs icon indicating copy to clipboard operation
dash-docs copied to clipboard

Mutable default arguments in AIO example 1

Open frnhr opened this issue 3 years ago • 1 comments

All-in-one components docs, example 1, gives this code (ellipsis mine):

class MarkdownWithColorAIO(html.Div):  # html.Div will be the "parent" component

    # A set of functions that create pattern-matching callbacks of the sub-components
    class ids:
        ...

    ...

    def __init__(
        self,
        text,
        colors=['#001f3f', '#0074D9', '#85144b', '#3D9970'],
        markdown_props={},
        dropdown_props={},
        aio_id=None
    ):
        ...

Aren't these default parameters for the __init__ method an example of the Mutable Default Parameters "gotcha", very common with Python beginners? My belief is that it should be changed to immutable versions, something like:

    def __init__(
        self,
        text,
        colors=('#001f3f', '#0074D9', '#85144b', '#3D9970'),
        markdown_props=None,
        dropdown_props=None,
        aio_id=None
    ):
        markdown_props = markdown_props or {}
        dropdown_props = dropdown_props or {}
        ...

frnhr avatar Jan 10 '22 17:01 frnhr

Thanks @frnhr - in fact even if the user gives values here, we don't want to mutate them, so further down there are lines:

dropdown_props = dropdown_props.copy()  # copy the dict so as to not mutate the user's dict

and similar for markdown_props

Which means there isn't actually a problem here, but you're right that it's better to use a flavor of the more robust pattern by leaving mutable objects out of the default slot. So perhaps combining the two lines into one:

dropdown_props = dropdown_props.copy() if dropdown_props else {}

and using None as the default is the way to go.

cc @LiamConnors

alexcjohnson avatar Jan 11 '22 21:01 alexcjohnson