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

Decorator Removed #1430

Closed
wants to merge 2 commits into from
Closed

Decorator Removed #1430

wants to merge 2 commits into from

Conversation

Patil2099
Copy link
Contributor

Fixes #1423

@Patil2099
Copy link
Contributor Author

@pipermerriam

@kclowes
Copy link
Collaborator

kclowes commented Aug 23, 2019

@Patil2099 We'll review once all the tests are passing! Thanks!

@Code0x58
Copy link
Contributor

Code0x58 commented Sep 6, 2019

dict_copy does more than work around def f(d={}): reusing the default parameter object.

From the tests (given that dict comparisons are failing), it looks like it is also being relied on to create deep copies of objects so that the code using it can mutate the object without affecting the original object.

It would probably be good to mark dict_copy as deprecated if nowhere in the codebase uses it, then make an issue for it and tag it with the next major release.

You will also need to find where copies are needed to avoid mutating dictionaries the function/code doesn't own. I think keeping on this approach would be better for the code in the long run rather than continuing to rely on dict_copy.

dict_copy has a few issues, including documentation and the name being wrong (they match the intent, but not what it actually does). For example it copies a lot more than just dicts, and any type of mutable object inside them (not just dict/list/tuple/set) will be deep copied. I suspect it didn't copy args just to avoid working around the first parameter of class methods. Killing it with fire sounds good to me.

@Patil2099 Patil2099 closed this Feb 12, 2021
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.

ens.utils.dict_copy only operates on keyword arguments
3 participants