Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Switch to using safe default arguments #1311

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Switch to using safe default arguments #1311

merged 1 commit into from
Mar 29, 2021

Conversation

r-barnes
Copy link
Contributor

@r-barnes r-barnes commented Mar 29, 2021 •
edited

When [] or {} or any other mutable type is used as a default argument, this can have unexpected results as discussed here and here for reasons explained here.

Rather than relying on programmer discipline not to abuse these default arguments, it is generally considered best practice to default value the arguments to None and check this within the function body.

This PR fixes all the instances I found, excluding the doc/ subdir, which I assume consists largely of upstream code.

@rileyjmurray
Copy link
Collaborator

@r-barnes thanks for catching this! I've run into this issue on other projects and also use the default-None approach. I'll merge this in once CI tests pass.

@rileyjmurray
Copy link
Collaborator

@r-barnes can you resolve the merge conflicts on your end?

@r-barnes
Copy link
Contributor Author

@rileyjmurray : Done.

SteveDiamond
SteveDiamond approved these changes Mar 29, 2021
View reviewed changes
Copy link
Collaborator

@SteveDiamond SteveDiamond left a comment

Choose a reason for hiding this comment

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

LGTM

@SteveDiamond
Copy link
Collaborator

Thanks for doing this!

@SteveDiamond SteveDiamond merged commit 4fa14f8 into cvxpy:master Mar 29, 2021
SteveDiamond added a commit that referenced this pull request Mar 30, 2021
@SteveDiamond
Merge branch 'master' into bugfix/value
4a35f77 
* master:
  Use lazy interpolation for logging (#1312)
  Classes do not need to inherit from object in Python3 (#1316)
  resolved #1315
  clarify code
  Switch to using safe default arguments (#1311)
  merging in #1313
@r-barnes r-barnes deleted the richard/safe_default_args branch April 15, 2021 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveDiamond SteveDiamond SteveDiamond approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@r-barnes @rileyjmurray @SteveDiamond