Skip to content

Async compatible redirect panel #1976

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 11 commits into from
Aug 6, 2024

Conversation

salty-ivy
Copy link
Member

@salty-ivy salty-ivy commented Jul 31, 2024

Description

async compatible RedirectsPanel.

Checklist:

  • I have added the relevant tests for this change.
  • I have added an item to the Pending section of docs/changes.rst.

@salty-ivy salty-ivy changed the title Add a new async aprocess_request method as common pattern in base panel class. Async compatible redirect panel Aug 3, 2024
Comment on lines 41 to 43
response = super().process_request(request)
if iscoroutine(response):
return self.aprocess_request(request)
Copy link
Member

Choose a reason for hiding this comment

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

This flow will call super().process_request twice. We probably want to avoid that. I have some ideas, but none that I know are great. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

since aprocess_request is exclusive to RedirectsPanel

  1. we can pass response from process_request in aprocess_request as an argument.
  2. or else we change the check from coroutine to checking request being an instance ASGIRequest.

Copy link
Member

Choose a reason for hiding this comment

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

My preference is option 1

Copy link
Member Author

Choose a reason for hiding this comment

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

I think 2. won't work either : )

@salty-ivy salty-ivy marked this pull request as ready for review August 4, 2024 06:30
@salty-ivy salty-ivy requested a review from tim-schilling August 4, 2024 06:30
@tim-schilling
Copy link
Member

Looks good! Thanks for working with me to arrive at this solution.

@tim-schilling tim-schilling merged commit 405f9f2 into django-commons:main Aug 6, 2024
25 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants