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

FIX: Don't default to list in method args #2518

Merged
merged 3 commits into from Feb 4, 2015
Merged

FIX: Don't default to list in method args #2518

merged 3 commits into from Feb 4, 2015

Conversation

longhotsummer
Copy link
Contributor

@longhotsummer longhotsummer commented Feb 4, 2015

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.
@xordoquy
Copy link
Collaborator

xordoquy commented Feb 4, 2015

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']
Copy link
Member

@tomchristie tomchristie Feb 4, 2015

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
Copy link
Member

@tomchristie tomchristie Feb 4, 2015

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

@tomchristie
Copy link
Member

tomchristie commented Feb 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.

@tomchristie tomchristie added the Bug label Feb 4, 2015
@tomchristie
Copy link
Member

tomchristie commented Feb 4, 2015

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?

@longhotsummer
Copy link
Contributor Author

longhotsummer commented Feb 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:

@detail_route
def foo(self, request, *args, **kwargs):

it just didn't show up. Digging showed me that the methods param was empty.

@tomchristie tomchristie added this to the 3.0.5 Release milestone Feb 4, 2015
@tomchristie
Copy link
Member

tomchristie commented Feb 4, 2015

Okay, so just the brackets around the if clause for visual clarity and then I'm happy for this to go in.
Thanks! 😄

tomchristie added a commit that referenced this pull request Feb 4, 2015
FIX: Don't default to list in method args
@tomchristie tomchristie merged commit 3b00824 into encode:master Feb 4, 2015
@tomchristie
Copy link
Member

tomchristie commented Feb 4, 2015

Loverly.

@xordoquy
Copy link
Collaborator

xordoquy commented Feb 4, 2015

Thanks for tracking this down, this is a very tricky Python side effect.

@longhotsummer
Copy link
Contributor Author

longhotsummer commented Feb 4, 2015

👍 it has caught me a few times

@maryokhin
Copy link
Contributor

maryokhin commented Feb 4, 2015

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants