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

Add drf-psq package to docs #7451

Merged
merged 2 commits into from
Sep 9, 2020
Merged

Add drf-psq package to docs #7451

merged 2 commits into from
Sep 9, 2020

Conversation

AminHP
Copy link
Contributor

@AminHP AminHP commented Aug 2, 2020

Please checkout our extension for the Django REST framework.
https://github.com/drf-psq/drf-psq/

@AminHP AminHP changed the title Add drf-psq package to docs Add drf-psq package to docs Aug 2, 2020
@AminHP
Copy link
Contributor Author

AminHP commented Aug 21, 2020

Anyone?!!

Copy link
Member

@rpkilby rpkilby left a comment

Choose a reason for hiding this comment

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

Hi @AminHP. I'm hesitant to merge this, as the description doesn't make much sense, specifically:

support for having view-based permission_classes, serializer_class, and queryset.

These are already attributes of the view, so they're already "view-based". Looking at the project, it looks like you've instead made querysets/serializers dependent on permission-based rules, with the ability to change these per handler (list, retrieve, create, etc..)?

Does that more accurately reflect what psq does?

@AminHP
Copy link
Contributor Author

AminHP commented Aug 26, 2020

Well, your description is what exactly psq does. But, I think in some way those attributes are not "view-based"; because when you have multiple handlers in a class, they can't have their own serializer/queryset/permission_class. So, they are somehow "class-based" and that's why I chose this description.

@rpkilby
Copy link
Member

rpkilby commented Aug 27, 2020

But, I think in some way those attributes are not "view-based"; because when you have multiple handlers in a class ...
So, they are somehow "class-based" and that's why I chose this description.

The description is at odds with established Django/DRF terminology. A View class is considered a view, even if it has handlers for multiple HTTP methods. So, saying that psq provides view-based attributes is incorrect. It might be better to instead emphasize that the psq enables per-action attributes for a viewset.

@AminHP
Copy link
Contributor Author

AminHP commented Aug 27, 2020

So, changing the description as below will solve the problem, right?

drf-psq is an extension for the Django REST framework that gives support for having action-based permission_classes, serializer_class, and queryset dependent on permission-based rules.

@AminHP
Copy link
Contributor Author

AminHP commented Sep 7, 2020

@rpkilby

@tomchristie
Copy link
Member

Yup, that would make much more sense from my point of view. 👍

@AminHP
Copy link
Contributor Author

AminHP commented Sep 7, 2020

Thanks for your review. We will apply the changes soon.

AminHP and others added 2 commits September 9, 2020 04:15
this package is an extension that gives support for having action-based **permission_classes**, **serializer_class**, and **queryset** dependent on permission-based rules.
@tomchristie tomchristie dismissed rpkilby’s stale review September 9, 2020 07:48

Requested changes now made.

@tomchristie tomchristie merged commit 04f39c4 into encode:master Sep 9, 2020
sigvef pushed a commit to sigvef/django-rest-framework that referenced this pull request Dec 3, 2022
* Add drf-psq to third party packages

* Add drf-psq to permissions.md

this package is an extension that gives support for having action-based **permission_classes**, **serializer_class**, and **queryset** dependent on permission-based rules.

Co-authored-by: Salar Nasiri <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants