search

holoviz / panel

Panel: The powerful data exploration & web app framework for Python
https://panel.holoviz.org
BSD 3-Clause "New" or "Revised" License
4.73k stars 516 forks source link

Remove mutable default args in function definitions #488

Closed mpkocher closed 5 years ago

mpkocher commented 5 years ago

There appears to be several cases where mutable args, such as a list or dict, are used as function defaults.

https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments

pylint panel | grep  dangerous-default-value
panel/viewable.py:406:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/util.py:118:0: W0102: Dangerous default value [] as argument (dangerous-default-value)
panel/param.py:345:12: W0102: Dangerous default value [] as argument (dangerous-default-value)
panel/pipeline.py:32:4: W0102: Dangerous default value [] as argument (dangerous-default-value)
panel/interact.py:126:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/interact.py:382:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/holoviews.py:176:4: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:83:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:123:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:170:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:178:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:186:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:244:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/pane/vtk/vtkjs_serializer.py:291:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/io/save.py:48:0: W0102: Dangerous default value {} as argument (dangerous-default-value)
panel/io/model.py:56:0: W0102: Dangerous default value [] as argument (dangerous-default-value)

Also, I believe it would be useful to add an autoformatting tool such as black or autopep8 to the commit/PR process when time permits.

jbednar commented 5 years ago

Thanks. I don't have any opinion pro or con about autoformatting; if it worked well with Parameter declarations then I'd be ok with it. Up to @philippjfr.

From what I can tell, most of those warnings could be avoided by replacing a declaration like def fn(x={}) with def fn(x=None); if x is None x={}, and that seems like a good idea. But at least two of them appear to me to be a very deliberate usage of the mutable argument to collect state across multiple calls, as noted below:

panel/viewable.py:406:4: Could replace with None
panel/util.py:118:0: Used deliberately to collect arguments in a recursive call
panel/param.py:345:12: Used deliberately to collect arguments in a recursive call
panel/pipeline.py:32:4: Could replace with None
panel/interact.py:126:4: Could replace with None
panel/interact.py:382:4: Could replace with None
panel/pane/holoviews.py:176:4: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:83:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:123:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:170:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:178:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:186:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:244:0: Could replace with None
panel/pane/vtk/vtkjs_serializer.py:291:0: Could replace with None
panel/io/save.py:48:0: Appears to be unused; could replace with None
panel/io/model.py:56:0: Could replace with None

If someone else could double-check my categorizations, then I'd be happy to see a PR where the two deliberate usages are marked with #noqa and the rest are replaced with None.

philippjfr commented 5 years ago

Thanks for the report. It would be nice to get rid of most of these. I tried to avoid the pattern of caching stuff in default kwargs but in certain cases it seems those stuck around. I don't think it's a huge priority to fix those instances so removing the rest and putting a # noqa around them seems reasonable to me.

I do like black but it doesn't play that nicely with param so I don't think we could adopt it. I haven't played with autopep8 to see how well that works but it's certainly worth investigating.

xtaje commented 5 years ago

Will submit as several PRS per package. First one is #692.

xtaje commented 5 years ago

692 and #694 should fix the items listed above, except for panel/param.py, which did looked non-trivial to verify by inspection.

philippjfr commented 5 years ago

Thanks for helping out @xtaje