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

Avoid mutable default arguments #553

Merged
merged 1 commit into from
Apr 25, 2017

Conversation

mbeacom
Copy link
Collaborator

@mbeacom mbeacom commented Apr 24, 2017

The goal of this PR is to remove mutable arguments that are defaulted in methods throughout the codebase.

For information on causation and manual steps to resolution, check out:
https://www.quantifiedcode.com/app/issue_class/3P0qV6OB

@mbeacom mbeacom added cleanup refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant. labels Apr 24, 2017
@mbeacom mbeacom self-assigned this Apr 24, 2017
@Zulko
Copy link
Owner

Zulko commented Apr 24, 2017

Nice ! was it actually done by a robot ??

@tburrows13
Copy link
Collaborator

Looks fine. For anyone wondering the purpose of this, see http://stackoverflow.com/questions/1132941/least-astonishment-and-the-mutable-default-argument

@mbeacom
Copy link
Collaborator Author

mbeacom commented Apr 24, 2017

@Zulko It was indeed! Not really something that I was hiding? 👍 No reason to take the time to go through and manually modify simple, but fairly important best practices that can be automagically fixed. We could have the same service resolve these directly if the integration is setup on the parent repo.

Some of these are fairly trivial to change manually, but should probably start being enforced against the master repo. Thoughts?

What do you think about adding https://www.quantifiedcode.com/ to the repo? It's free for opensource projects!

Unless you want more one-liners to resolve this like apply_to = apply_to or [], but this can lead to failures pending on the passed argument types. Realistically it shouldn't. If you'd prefer that, I'll resubmit with manual changes.

@coveralls
Copy link

coveralls commented Apr 24, 2017

Coverage Status

Coverage increased (+0.07%) to 54.463% when pulling 34db3d3 on mbeacom:autofix/wrapped2_to3_fix-0 into 03fdc2c on Zulko:master.

@mbeacom mbeacom closed this Apr 25, 2017
@tburrows13
Copy link
Collaborator

Any reason why this is closed @mbeacom?

@mbeacom mbeacom reopened this Apr 25, 2017
@mbeacom mbeacom merged commit 7bd0678 into Zulko:master Apr 25, 2017
@mbeacom mbeacom added this to the Release v0.2.3.3 milestone Apr 25, 2017
@mbeacom
Copy link
Collaborator Author

mbeacom commented Apr 25, 2017

@Gloin1313 I was uncertain if there were objections to the changes. XD

@ghost
Copy link

ghost commented Apr 25, 2017

@mbeacom I definitely see this PR as a enhancement..

@Zulko
Copy link
Owner

Zulko commented Apr 25, 2017

LGTM

@mbeacom mbeacom added the enhancement Positive change that does not change the API, i.e. improved performance, using less memory etc. label Apr 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Positive change that does not change the API, i.e. improved performance, using less memory etc. refactor Does not affect the end user at all i.e. making code easier to read or PEP8 compliant.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants