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

Release/3.0.0 #820

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Release/3.0.0 #820

wants to merge 9 commits into from

Conversation

tomwojcik
Copy link
Contributor

@tomwojcik tomwojcik commented May 4, 2024

todos:

  • update docs
  • update comments / docstrings, if needed
  • move tests to upstream so the diff is smaller and more reliable (unused mocks etc)
  • some tests are mediocre at best. Compare the copy-pasted post methods body vs upstream ModelViewSet actions
  • in the upstream add tests to check the resolved view paths to guarantee they did not change after the migration

@tomwojcik tomwojcik requested a review from haxoza May 4, 2024 14:49
@tomwojcik tomwojcik self-assigned this May 4, 2024
User = get_user_model()


class UserViewSet(viewsets.ModelViewSet):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should it be split further?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to split completely small views.

I think the whole point of splitting is that you can easily get any view, extend it or modify its behavior and inject to your project ONLY what you want.

With this viewset if you don't want to have e.g. update code in you project you can only remove the url pattern, but not the view handler.

Taking a lesson from the community feedback, I would say that viewset is not the best choice for a library.

@@ -72,8 +73,11 @@ def test_user_can_get_other_user_detail(self):

def test_user_cant_set_other_user_detail(self):
login_user(self.client, self.user)
response = self.client.get(reverse("user-detail", args=[self.superuser.pk]))
self.assert_status_equal(response, status.HTTP_200_OK)
response = self.client.patch(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test body copied from above (dupe), did not match the test name

Copy link
Member

@haxoza haxoza left a comment

Choose a reason for hiding this comment

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

Overall looks good, however I'm in favor of further splitting!

re_path(r"^token/login/?$", views.TokenCreateView.as_view(), name="login"),
re_path(r"^token/logout/?$", views.TokenDestroyView.as_view(), name="logout"),
re_path(r"^token/login/?$", TokenCreateView.as_view(), name="login"),
re_path(r"^token/logout/?$", TokenDestroyView.as_view(), name="logout"),
Copy link
Member

Choose a reason for hiding this comment

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

We may want to switch to url() instead of re_path by the way of doing this views split.

User = get_user_model()


class UserViewSet(viewsets.ModelViewSet):
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to split completely small views.

I think the whole point of splitting is that you can easily get any view, extend it or modify its behavior and inject to your project ONLY what you want.

With this viewset if you don't want to have e.g. update code in you project you can only remove the url pattern, but not the view handler.

Taking a lesson from the community feedback, I would say that viewset is not the best choice for a library.

@tomwojcik tomwojcik added this to the 3.0.0 milestone Jun 7, 2024
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.

2 participants