PR: Remove mutable defaults for keyword arguments #23293
Conversation
ccordoba12
changed the title
ccordoba12
left a comment
ccordoba12
left a comment
There was a problem hiding this comment.
Thanks for your work on this @gb119!
| providers = [] if providers is None else providers | ||
| super().__init__(plugin, parent) |
There was a problem hiding this comment.
| providers = [] if providers is None else providers | |
| super().__init__(plugin, parent) | |
| super().__init__(plugin, parent) | |
| providers = [] if providers is None else providers |
Our custom is not add code before calling super unless it's really necessary.
| def __init__(self, parent=None, commands=None, message=None, | ||
| max_line_count=300, exitfunc=None, profile=False, | ||
| multithreaded=True): | ||
| commands = [] if commands is None else commands |
There was a problem hiding this comment.
Please move this to be at line 171 below.
| file_associations = ( | ||
| {} if file_associations is None else file_associations | ||
| ) |
There was a problem hiding this comment.
Please move these to be at line 321 below.
| external_path_history = ( | ||
| [] if external_path_history is None else external_path_history | ||
| ) |
There was a problem hiding this comment.
Please move these to be at line 77 below.
There was a problem hiding this comment.
Actually, this can just be removed as there is already a test for an empty list that works equally well for None
| data = [] if data is None else data | ||
| QAbstractTableModel.__init__(self, parent) |
There was a problem hiding this comment.
| data = [] if data is None else data | |
| QAbstractTableModel.__init__(self, parent) | |
| QAbstractTableModel.__init__(self, parent) | |
| data = [] if data is None else data |
| """ | ||
| args = [] if args is None else args | ||
| p_args = [] if p_args is None else p_args | ||
| assert module is not None |
There was a problem hiding this comment.
| assert module is not None | |
| assert module is not None |
For code clarity.
| parent, text, icon, package, module, args=None | ||
| ): | ||
| """Create action to run a GUI based Python script""" | ||
| args = [] if args else args |
There was a problem hiding this comment.
| args = [] if args else args | |
| args = [] if args is None else args |
It seems missing.
|
|
||
| def setup(self, options={}): | ||
| def setup(self, options=None): | ||
| options = {} if options is None else options |
There was a problem hiding this comment.
| options = {} if options is None else options | |
| options = {} if options is None else options | |
For clarity.
|
@gb119, I asked @jsbautista to take your contributions and open another PR with them (see #24337). We think you submitted some valuable contributions, so we don't want to lose them. Hence, I'm going to close this PR as superseded by that one. |
3 participants
Description of Changes
Whilst tracking down an odd race condition in locating conda, I spotted several instances where a mutable type
was being used as a default for a keyword argument. This PR simple replaces the mutable constant (typically [] and {})
with a
Noneand then does anx = [] if x is None else xor equivalent on the first line of each affected function.It is possible that functionality was depending on the default mutable value being changed by a function call, but
I'd argue that it is sufficiently non-obvious that doing that should be considered a bug :-)
Issue(s) Resolved
Partially Fixes #23061
Affirmation
By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.
I certify the above statement is true and correct: gb119 (https://github.com/gb119)