Removed mutable argument, potential latent bug #93
Conversation
Removed mutable arg exposing a latent bug
| return self.client.delete(self._url()) | ||
|
|
||
| def get(self, id, fields=[], include_fields=True): | ||
| def get(self, id, fields=None, include_fields=True): |
There was a problem hiding this comment.
So I'm not a python dev, wanted to understand the potential impact of this?
The default was an empty array, it is now None.
However the first thing you then do is check if fields was None then set it to an empty array. Which was the previous default anyway?
I also noticed other methods use [] for this param and you have not touched those?
https://github.com/Brett55/auth0-python/blob/e300ee2185df0bccd42b77751bda57b90cd7ed3e/auth0/v3/management/users.py#L28
There was a problem hiding this comment.
@cocojoe I didn't fix them all, I only fixed the one I saw while using the SDk. This link explains why using a list is bad http://docs.python-guide.org/en/latest/writing/gotchas/
There was a problem hiding this comment.
Thanks for the best practice link. Will review.
|
yes
…On Fri, Mar 23, 2018 at 9:19 AM, Luciano Balmaceda ***@***.*** > wrote:
***@***.**** commented on this pull request.
------------------------------
In auth0/v3/management/users.py
<#93 (comment)>:
> @@ -79,7 +79,7 @@ def delete_all_users(self):
"""
return self.client.delete(self._url())
- def get(self, id, fields=[], include_fields=True):
+ def get(self, id, fields=None, include_fields=True):
I only fixed the one I saw while using the SDk
Would be nice if this PR updates every missing lines just like the one
@cocojoe <https://github.com/cocojoe> linked. Are you willing to update
it @Brett55 <https://github.com/brett55> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#93 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGsE2TbherhbRTO1cj0lm7rosnCz0i2Tks5thRJsgaJpZM4Sw6EQ>
.
|
|
I'm closing this due to inactivity. I've added a new PR solving all the occurrences. Thanks for your contribution. |
3 participants
I removed a mutable argument (list) as it holds state between function/method calls. This is a latent bug that could be exposed down the road.