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.

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

Cleans up dangerous function default variables ([] and {}) #2275

Merged
merged 1 commit into from
Nov 4, 2017

Conversation

sklarsa
Copy link
Contributor

@sklarsa sklarsa commented Nov 2, 2017

Description

Changes function default variables {} -> dict() and [] -> list()

Motivation and Context

Just cleaning up some of the errors from https://landscape.io/github/spotify/luigi/1347/messages/error

Have you tested this? If so, how?

Ran tox -e py27-nonhdfs and everything passes

@dlstadther
Copy link
Collaborator

Thanks @sklarsa ! The errors here are waiting on #2274 for resolution

@sklarsa
Copy link
Contributor Author

sklarsa commented Nov 3, 2017

Sounds good. Let me know if you need anything else from my end

@dlstadther dlstadther merged commit 1787d82 into spotify:master Nov 4, 2017
@dlstadther
Copy link
Collaborator

Thanks @sklarsa !

@jhorman
Copy link
Contributor

jhorman commented Dec 5, 2017

Isn't this still dangerous. The default args are still mutable. Isn't the right approach to set these to None, and default in code.

@kwilcox
Copy link
Contributor

kwilcox commented Dec 5, 2017

Correct @jhorman, this did not fix what was intended. However, I took a quick look and none of the mutable arguments are changed within the functions. It would be better to default these to None and set the defaults accordingly inside of the function as you mentioned but this shouldn't be causing any issues as is.

@Tarrasch
Copy link
Contributor

Tarrasch commented Dec 5, 2017

@sklarsa, can you please send a new PR fixing this as @jhorman pointed out?

@sklarsa
Copy link
Contributor Author

sklarsa commented Dec 5, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants