Skip to content

Handle "suffix" used in action decorator kwargs #6079

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Handle "suffix" used in action decorator kwargs #6079

wants to merge 3 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jul 10, 2018 •
edited
Loading

With 0148a9f you would get a TypeError when using "suffix" in an
action decorator.

This might get improved to take suffix as a keyword argument already,
defaulting to None.

TODO:

  • test

/cc @rpkilby

@blueyed
Handle "suffix" used in action decorator kwargs
cefc478 
With 0148a9f you would get a TypeError when using "suffix" in an
action decorator.

This might get improved to take suffix as a keyword argument already,
defaulting to `None`.
@codecov-io
Copy link

codecov-io commented Jul 10, 2018 •
edited
Loading

Codecov Report

Merging #6079 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@rpkilby rpkilby self-requested a review July 10, 2018 13:53
@rpkilby rpkilby self-assigned this Jul 10, 2018
@rpkilby
Copy link
Member

rpkilby commented Jul 10, 2018

Were you previously using suffix with the action decorator, or is this more a hypothetical conflict?

I haven't fully thought this out, but my initial reaction is to just disallow the suffix argument, instead requiring users to provide the full name. If we were to all suffixes, the name and suffix argument would need to be mutually exclusive, similar to how they're handled in ViewSet. The func.name would also need to be set to None. I'm not sure if it's worth the extra complexity? Again, my thoughts aren't fully formed on this - feel free to push back.

django-rest-framework/rest_framework/viewsets.py

Lines 90 to 93 in 0148a9f

# name and suffix are mutually exclusive
if 'name' in initkwargs and 'suffix' in initkwargs:
raise TypeError("%s() received both `name` and `suffix`, which are "
"mutually exclusive arguments." % (cls.__name__))

@blueyed
Copy link
Contributor Author

blueyed commented Jul 10, 2018 •
edited
Loading

Were you previously using suffix with the action decorator

Yes.

my initial reaction is to just disallow the suffix argument

We're using it in a mixin, so it is not trivial to use the full name there.

@rpkilby
Copy link
Member

rpkilby commented Jul 10, 2018

We're using it in a mixin, so it is not trivial to use the full name there.

That makes sense. I would say it's worth allowing then.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 10, 2018

Cool.
I think this is ready then to be squash-merged.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 10, 2018

Or do you think it should be added to the signature already?

@rpkilby
Copy link
Member

rpkilby commented Jul 10, 2018

So there are some more difficulties with this. While the PR fixes the exception, there's still the discrepancy in name vs suffix. e.g., you'll notice that the dropdown uses the name, while the breadcrumbs use the suffix.

I'm currently working on a fix.

@rpkilby rpkilby mentioned this pull request Jul 10, 2018
Merged
3 tasks
@rpkilby
Copy link
Member

rpkilby commented Jul 13, 2018

Hi @blueyed - closing this in favor of #6081

@rpkilby rpkilby closed this Jul 13, 2018
@rpkilby rpkilby removed their request for review July 13, 2018 14:55
@blueyed blueyed deleted the fix-suffix-from-action branch July 13, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@rpkilby rpkilby

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blueyed @codecov-io @rpkilby