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

Conversation24 Commits13 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 }})

Pulling a slightly more manageable chunk out of #5605 for review. The goal is to remove logic from the router in order to make the viewsets and list/detail actions more introspectable.

Changes:

Deprecated list_route & detail_route in favor of action decorator w/ detail boolean.
Deprecated dynamic list/detail route variants in favor of DynamicRoute w/ detail boolean.
Added detail boolean argument to regular Route
A route's detail is now provided as an initkwarg to the ViewSet, similar to suffix.
Added ViewSet.get_extra_actions() to formalize extra action access.
Added url_path and url_name to action signature. These values are no longer set/altered by the router, and can be more reliably inspected by the parent viewset.
Refactored dynamic route generation, replacing protected method w/ internal instance method.

For the release notes, I've added a section to discuss the changes as a larger feature.

TODO:

Add release notes
Replace deprecated functions in tests
Update docs w/ changes

Ryan P Kilby added 6 commits

December 22, 2017 01:18

rpkilby added this to the 3.8 Release milestone

Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is nicely done. Good benefits. Worth having.

@rpkilby, again, thanks for the effort. If you can tick off the TODOs lets have it. 👍

* Merged `list_route` and `detail_route` into a single `action` decorator.
* Get all extra actions on a `ViewSet` with `.get_extra_actions()`.
* Extra actions now set the `url_name` and `url_path` on the decorated method.
* Enable action url reversing through `.reverse_action()` method (added in 3.6.4)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...3.7.4...

Ryan P Kilby added 2 commits

January 4, 2018 02:55

Okay, this should be ready to go now. The docs changes are not completely trivial and feedback would be appreciated.


class UserViewSet(ModelViewSet):
...

@detail_route(methods=['post'], permission_classes=[IsAdminOrIsSelf])
@action(methods=['post'], detail=True, permission_classes=[IsAdminOrIsSelf])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point the detail parameter is mysterious. (Worth explaining it below the example.)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be sufficient to link to the viewset docs first? e.g., "go read this first, then come back here"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. (I was thinking along those lines... 🙂)

Perhaps moving the note from ln124 up and into a Note: section may be worthwhile. (Who reads about Routers before ViewSets?)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the docs. Integrated line 124 into the start of the section.

@@ -130,29 +130,45 @@ def decorator(func):
return decorator


def detail_route(methods=None, **kwargs):
def action(methods=None, detail=True, url_path=None, url_name=None, **kwargs):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the detail parameter...:

Do we want an implied default? Or do we want to make it required, with a helpful assertion?
If implied which is the best default? i.e. Do we generate list or detail routes if you don't pass the parameter.
Do we want to document whichever we choose as not passing the parameter? @action(detail=True) and @action() in all the examples, say?

Not sure what I think there.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want an implied default? Or do we want to make it required, with a helpful assertion?

I don't think a default is beneficial here. Making it a required arg seems 👍 to me.

Ryan P Kilby added 2 commits

January 8, 2018 07:30

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. This looks good.

I'm just going to pause on this for now, since once it's in we're kind of committed to rolling a 3.8. (which is fine, but I just want to plan that first. 🙂) //cc @tomchristie

It seems that there are some regression on list_route & detail_route.
If no url_name was provided, it used to default on the url_path:

url_name = initkwargs.pop("url_name", None) or url_path

With the new code, it default to the function name:

func.url_name = url_name or func.__name__.replace('_', '-')

which breaks a couple of tests here and there.

Hey @xordoquy. Would you be able to put a regression test together for that?

need to investigate another issue and I'll do that.

Hi @xordoquy - this was an intentional change, as the url_path is not always a suitable default for the name. eg, when the path contains URL parameters.

Also, sorry for being afk lately - life has been pretty busy the last couple of months.

that's ok, but this should be added to the breaking changes since it requires changes on detail_route and list_route.

Maybe the path forward is for action to have this new behavior, with list_route and detail_route maintaining the old behavior.

I need to have a look at exactly what’s going on here but could we not handle the change in the shim?

@rpkilby you must not apologise for life. Contribution to Open Source needs to be scoped. There is no problem with that. Ever. The only problem is with the opposite. 🙂

Thanks - just letting you know I'm still around.

Maybe the path forward is for action to have this new behavior, with list_route and detail_route maintaining the old behavior.

sounds sensible to me.

Also, sorry for being afk lately

Didn't react but seconding @carltongibson on that point, you don't need to apologize for that. You did a lot of work so far, and for that we owe you one.

v.3.8.1 restores the old behaviour (via #5915). It's on PyPI now.

Thanks you for the fix and the prompt release of v3.8.1

nijel added a commit to WeblateOrg/weblate that referenced this pull request

Apr 18, 2018

This comment has been minimized.

@Allan-Nava Please direct usage questions to Stack Overflow or the mailing list. (Again, the issue tracker is for bug reports.) Thanks.

pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request

Nov 17, 2020

Merge list/detail route decorators into 'action'
Merge dynamic routes, add 'detail' attribute
Add 'ViewSet.get_extra_actions()'
Refactor dynamic route checking & collection
Refactor dynamic route generation
Add 'ViewSet.detail' initkwarg
Fixup schema test
Add release notes for dynamic action changes
Replace list/detail route decorators in tests
Convert tabs to spaces in router docs
Update docs
Make 'detail' a required argument of 'action'
Improve router docs