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

ens.utils.dict_copy only operates on keyword arguments #1423

Closed
pipermerriam opened this issue Aug 19, 2019 · 5 comments
Closed

ens.utils.dict_copy only operates on keyword arguments #1423

pipermerriam opened this issue Aug 19, 2019 · 5 comments

Comments

@pipermerriam
Copy link
Member

What was wrong?

There is a dict_copy decorator used in the ens module. It is intended to allow us to write methods like this:

@dict_copy
def my_method(self, some_params={}):
    ...

Normally using a mutable value like a dictionary as a default argument is problematic because the same mutable object is used across each call. This decorator institutes a copy of this, but only if it is passed as a keyword argument.

There are at least some places where dict_copy is used but where the parameter that needs to be copied is not enforced to be a keyword argument, meaning that any calls that use it positionally will not benefit from the protection.

How can it be fixed?

I'm inclined to suggest removing the decorator entirely and changing all places it was used to follow the standard pattern of:

def my_method(self, some_params=None):
    if some_params is None:
        some_params = {}
    ...
@Patil2099
Copy link
Contributor

Can I work on it? @pipermerriam?

@pipermerriam
Copy link
Member Author

@Patil2099 👍

@shashankks0987
Copy link

I'd like to take this if it is still open!

@dbfreem
Copy link
Contributor

dbfreem commented Dec 1, 2021

So I was thinking about taking this one on but then started thinking. Wouldn't this be a breaking change as consumers of the library could be relying on the @dict_copy to not mutate the dictionary that they are passing in to things like main.setup_address. I can make the change but just wanted to make sure I made it on the right branch as it might not be appropriate to make the change on master.

@pacrob
Copy link
Contributor

pacrob commented Feb 4, 2022

Fixed in PR #1998

@pacrob pacrob closed this as completed Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants