Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

remove mutable default arguments #93

Merged
merged 4 commits into from
Jun 9, 2020
Merged

remove mutable default arguments #93

merged 4 commits into from
Jun 9, 2020

Conversation

jurasofish
Copy link
Contributor

Mutable default arguments are a common gotcha. They might not be causing issues currently, but they are a time bomb likely to result in insidious issues as the code around them changes.

Copy link
Contributor

@h-g-s h-g-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure If I understood the purpose of these changes. Wouldn't it make a bit slower an additional check ?

@jurasofish
Copy link
Contributor Author

Yeah it might make it a tad slower due to additional checks in some cases, though that's not the case with this pull request any longer.

The issue is that

  • a lot of people don't understand how Python behaves with mutable default arguments
  • mutable default args typically result in unintended behaviour.
  • If code relies on mutable default args that's likely extremely poor quality code.

The changes in this PR aren't addressing a pressing problem, just following best practices and improving the robustness of the code.

For example,

>>> def f(a=[]):
...     a.append(1)
...     print(a)
...     
>>> f([1,2,3])
[1, 2, 3, 1]
>>> f()
[1]
>>> f()
[1, 1]

@sebheger
Copy link
Collaborator

sebheger commented Jun 8, 2020

Please correct if I am wrong, but I believe, that this change wouldn't harm the performance. The code @jurasofish proposed here is start-of-the-art python programming.

@h-g-s h-g-s merged commit 7ffd6ae into coin-or:master Jun 9, 2020
@h-g-s
Copy link
Contributor

h-g-s commented Jun 9, 2020

jGaboardi pushed a commit to jGaboardi/python-mip that referenced this pull request Jul 28, 2020
* remove mutable default arguments

* lint

* remove mutable default args

Former-commit-id: 7ffd6ae
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants