FIX: Don't default to list in method args by longhotsummer · Pull Request #2518 · encode/django-rest-framework (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation12 Commits3 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
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.
Good catch though I'd probably rather have them as a tuple to avoid mutability.
| """ |
|---|
| Used to mark a method on a ViewSet that should be routed for detail requests. |
| """ |
| if methods is None: |
| methods = ['get'] |
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 the methods = ['get'] if (methods is None) else methods style.
| """ |
|---|
| Used to mark a method on a ViewSet that should be routed for detail requests. |
| """ |
| methods = ['get'] if methods is None else methods |
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 methods
I'll take this either with or without tuples. Happy for that to be discussed separately as it's a slightly different issue.
In some cases the list gets cleared
This fix is clearly correct, but I'd be interested to know in what context the list gets mutated?
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:
@detail_route
def foo(self, request, *args, **kwargs):
it just didn't show up. Digging showed me that the methods param was empty.
tomchristie added a commit that referenced this pull request
FIX: Don't default to list in method args
Thanks for tracking this down, this is a very tricky Python side effect.
👍 it has caught me a few times
it just didn't show up.
It did work for me if used as @detail_route(), but not as @detail_route. I even thought that I didn't understand how decorators work in Python for a while, and that's how it should be.
This was referenced
Mar 9, 2017
This was referenced
Oct 6, 2017
This was referenced
Oct 16, 2017
This was referenced
Nov 6, 2017
This was referenced
Nov 14, 2017
This was referenced
Dec 10, 2017
This was referenced
Dec 20, 2017