Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
FIX: Don't default to list in method args#2518
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Fixes @list_route and @detail_route so that they don't initialize their `methods` parameter as a list. In some cases the list gets cleared, and the result is that default parameter is now empty, and may get reused unexpectedly.
xordoquy commentedFeb 4, 2015
Good catch though I'd probably rather have them as a tuple to avoid mutability. |
rest_framework/decorators.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add a newline here, also let's use themethods = ['get'] if (methods is None) else methods style.
rest_framework/decorators.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor one, this, but I tend to prefer using brackets around the if clause for visual clarity, so I normally do...
['get'] if (methods is None) else methodslovelydinosaur commentedFeb 4, 2015
I'll take this either with or without tuples. Happy for that to be discussed separately as it's a slightly different issue. |
lovelydinosaur commentedFeb 4, 2015
This fix is clearly correct, but I'd be interested to know in what context the list gets mutated? |
longhotsummer commentedFeb 4, 2015
Regarding tuples, I chose not to use them because I didn't want to mess with any code which is trying to change the list (even if it shouldn't be), I didn't want to go down that path right now. I'm using a pretty standard setup and I just happened to notice that if I used: it just didn't show up. Digging showed me that the methods param was empty. |
lovelydinosaur commentedFeb 4, 2015
Okay, so just thebrackets around the if clause for visual clarity and then I'm happy for this to go in. |
FIX: Don't default to list in method args
lovelydinosaur commentedFeb 4, 2015
Loverly. |
xordoquy commentedFeb 4, 2015
Thanks for tracking this down, this is a very tricky Python side effect. |
longhotsummer commentedFeb 4, 2015
👍 it has caught me a few times |
maryokhin commentedFeb 4, 2015
It did work for me if used as |
Fixes
@list_routeand@detail_routeso that they don't initialize theirmethodsparameter as a list. In some cases the list gets cleared, and the result is that default parameter is now empty, and may get reused unexpectedly.