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:

Deprecatedlist_route &detail_route in favor ofaction decorator w/detail boolean.
Deprecated dynamic list/detail route variants in favor ofDynamicRoute w/detail boolean.
Addeddetail boolean argument to regularRoute
A route'sdetail is now provided as aninitkwarg to the ViewSet, similar tosuffix.
AddedViewSet.get_extra_actions() to formalize extra action access.
Addedurl_path andurl_name toaction 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 added6 commits

December 22, 2017 01:18

Merge list/detail route decorators into 'action'

f79cd59

Merge dynamic routes, add 'detail' attribute

e090ba2

Add 'ViewSet.get_extra_actions()'

72b7379

Refactor dynamic route checking & collection

c2476fc

Refactor dynamic route generation

034f8d0

Add 'ViewSet.detail' initkwarg

fed7616

rpkilby added this to the3.8 Release milestone

Dec 22, 2017

rpkilby force-pushed theaction-decorator branch fromec92bc3 to046bf8aCompare

December 22, 2017 17:36

Fixup schema test

23447b0

rpkilby force-pushed theaction-decorator branch from87ba917 to23bf8ccCompare

December 22, 2017 17:49

Add release notes for dynamic action changes

a591e16

carltongibson reviewed

Jan 2, 2018

View reviewed changes

Copy link

Collaborator

carltongibson left a comment

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. 👍

docs/topics/release-notes.md Outdated

		* 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)

Copy link

Collaborator

carltongibsonJan 2, 2018

Choose a reason for hiding this comment

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

...3.7.4...

rpkilby force-pushed theaction-decorator branch 2 times, most recently from96b3531 toa591e16Compare

January 3, 2018 23:15

Replace list/detail route decorators in tests

151c0c9

rpkilby force-pushed theaction-decorator branch 2 times, most recently from01f568f to151c0c9Compare

January 3, 2018 23:49

Ryan P Kilby added2 commits

January 4, 2018 02:55

Convert tabs to spaces in router docs

b69efe0

Update docs

2112a58

Copy link

ContributorAuthor

rpkilby commentedJan 4, 2018

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

carltongibson reviewed

Jan 4, 2018

View reviewed changes

docs/api-guide/routers.md

		...

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

Copy link

Collaborator

carltongibsonJan 4, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

rpkilbyJan 4, 2018

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"

Copy link

Collaborator

carltongibsonJan 4, 2018

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 aNote: section may be worthwhile. (Who reads about Routers before ViewSets?)

Copy link

ContributorAuthor

rpkilbyJan 8, 2018

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.

carltongibson reviewed

Jan 4, 2018

View reviewed changes

rest_framework/decorators.py Outdated



		defdetail_route(methods=None,**kwargs):
		defaction(methods=None,detail=True,url_path=None,url_name=None,**kwargs):

Copy link

Collaborator

carltongibsonJan 4, 2018

Choose a reason for hiding this comment

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

So, thedetail 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 generatelist ordetail 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.

Copy link

ContributorAuthor

rpkilbyJan 8, 2018

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 added2 commits

January 8, 2018 07:30

Make 'detail' a required argument of 'action'

40ab128

Improve router docs

4f6190d

carltongibson approved these changes

Jan 19, 2018

View reviewed changes

Copy link

Collaborator

carltongibson left a comment

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

carltongibson merged commit73203e6 intoencode:master

Jan 25, 2018

rpkilby deleted the action-decorator branch

January 25, 2018 08:41

rpkilby mentioned this pull request

Jan 25, 2018

Improvements to ViewSet extra actions#5605

Merged

7 tasks

carltongibson mentioned this pull request

Feb 15, 2018

ImportError: cannot import name 'action'#5830

Closed

6 tasks

Copy link

Contributor

xordoquy commentedApr 4, 2018

It seems that there are some regression on list_route & detail_route.
If nourl_name was provided, it used to default on theurl_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.

Copy link

Collaborator

carltongibson commentedApr 4, 2018

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

Copy link

Contributor

xordoquy commentedApr 4, 2018

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

Copy link

ContributorAuthor

rpkilby commentedApr 4, 2018

Hi@xordoquy - this was an intentional change, as theurl_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.

Copy link

Contributor

xordoquy commentedApr 4, 2018

that's ok, but this should be added to the breaking changes since it requires changes ondetail_route andlist_route.

Copy link

ContributorAuthor

rpkilby commentedApr 4, 2018

Maybe the path forward is foraction to have this new behavior, withlist_route anddetail_route maintaining the old behavior.

Copy link

Collaborator

carltongibson commentedApr 4, 2018

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

Copy link

Collaborator

carltongibson commentedApr 4, 2018

@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. 🙂

Copy link

ContributorAuthor

rpkilby commentedApr 4, 2018

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

Copy link

Contributor

xordoquy commentedApr 4, 2018•
edited
Loading

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.

Copy link

Collaborator

carltongibson commentedApr 4, 2018

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

rpkilby mentioned this pull request

Apr 4, 2018

Use old url_name behavior in route decorators#5915

Merged

ding1992 mentioned this pull request

Apr 5, 2018

Incompatibility with drf 3.8.1chibisov/drf-extensions#211

Closed

Copy link

Contributor

jaap3 commentedApr 5, 2018

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

michael-k mentioned this pull request

Apr 5, 2018

docs: Add missing argument 'detail' to Route#5920

Merged

rpkilby mentioned this pull request

Apr 12, 2018

Fix router compatibility with DRF 3.8chibisov/drf-extensions#214

Merged

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

Apr 18, 2018

Raise rest_framework dependency to 3.8

907e397

This is needed to avoid deprecation warnigs on recent versions,seeencode/django-rest-framework#5705Signed-off-by: Michal Čihař <michal@cihar.com>

This comment has been minimized.

Copy link

Collaborator

carltongibson commentedApr 23, 2018

@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

Rework dynamic list/detail actions (encode#5705)

3c9d16c

* 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

Stefs-2142 mentioned this pull request

Dec 26, 2020

changed deprecated decorator @detail_route to actual @actionilyachch/django-rest-framework-rusdoc#21

Merged

Labels

None yet

Movatterモバイル変換

Uh oh!

Rework dynamic list/detail actions#5705

Rework dynamic list/detail actions#5705

Uh oh!

Conversation

rpkilby commentedDec 22, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

carltongibsonJan 2, 2018

Choose a reason for hiding this comment

Uh oh!

rpkilby commentedJan 4, 2018

Uh oh!

carltongibsonJan 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpkilbyJan 4, 2018

Choose a reason for hiding this comment

Uh oh!

carltongibsonJan 4, 2018

Choose a reason for hiding this comment

Uh oh!

rpkilbyJan 8, 2018

Choose a reason for hiding this comment

Uh oh!

carltongibsonJan 4, 2018

Choose a reason for hiding this comment

Uh oh!

rpkilbyJan 8, 2018

Choose a reason for hiding this comment

Uh oh!

carltongibson left a comment

Choose a reason for hiding this comment

Uh oh!

xordoquy commentedApr 4, 2018

Uh oh!

carltongibson commentedApr 4, 2018

Uh oh!

xordoquy commentedApr 4, 2018

Uh oh!

rpkilby commentedApr 4, 2018

Uh oh!

xordoquy commentedApr 4, 2018

Uh oh!

rpkilby commentedApr 4, 2018

Uh oh!

carltongibson commentedApr 4, 2018

Uh oh!

carltongibson commentedApr 4, 2018

Uh oh!

rpkilby commentedApr 4, 2018

Uh oh!

xordoquy commentedApr 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

carltongibson commentedApr 4, 2018

Uh oh!

jaap3 commentedApr 5, 2018

Uh oh!

This comment has been minimized.

carltongibson commentedApr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rpkilby commentedDec 22, 2017•
edited
Loading

carltongibsonJan 4, 2018•
edited
Loading

xordoquy commentedApr 4, 2018•
edited
Loading