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.

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

Use asgiref when available instead of thread locals (#176) #184

Closed
wants to merge 1 commit into from
Closed

Use asgiref when available instead of thread locals (#176) #184

wants to merge 1 commit into from

Conversation

darwing1210
Copy link

No description provided.

@darwing1210
Copy link
Author

@microsoft-github-policy-service agree company="newsamp"

@darwing1210 darwing1210 marked this pull request as ready for review June 1, 2023 00:51
@mjvankampen
Copy link

@agedemenli this seems like a nice fix for #176

@gurkanindibay
Copy link
Contributor

Hey @darwing1210
Here in your solution, the default implementation is asgiref.local
We want to keep the backward compatibility.
Could you add the support with a configuration parameter, which has the default value of threading.local?
Additionally, we need a test to test both cases.
To test the feature, you need to install asgiref package as well with the command pip install asgiref for asgiref test case

Thanks for your contribution @darwing1210

gurkanindibay
gurkanindibay requested changes Jul 11, 2023
View reviewed changes
Copy link
Contributor

@gurkanindibay gurkanindibay left a comment

Choose a reason for hiding this comment

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

Review notes are above @darwing1210

@darwing1210
Use asgiref when available instead of thread locals (citusdata#176)
3465bfc 
add asgiref as test dependency

add tests for asgiref

add setting TENANT_USE_ASGIREF, updated tests
@darwing1210
Copy link
Author

@gurkanindibay I updated the PR with your notes, but I personally think that the setting is not necessary as asgiref is included as a Django Requirement since >= 3, please let me know any comments/improvements.

@mjvankampen
Copy link

@gurkanindibay any chance to get this merged?

@gurkanindibay
Copy link
Contributor

@mjvankampen sorry for the delay. We have lots of things on our plate. We've recently added support for Django 4.1 and 4.2 in PR #197
I'm waiting it to be merged tomorrow. After merging this PR, we can test with 4.1 and 4.2 as well.
Thanks for your contributions

@gurkanindibay
Copy link
Contributor

gurkanindibay commented Sep 26, 2023 •
edited
Loading

@darwing1210 @mjvankampen There were conflicts about the dependencies. Since I could not push to the @darwing1210's fork repo, I needed to open a new PR.
Could you check this PR?
#198

@gurkanindibay
Copy link
Contributor

Addressed with #198 Thanks @darwing1210 @mjvankampen for your contributions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjvankampen mjvankampen mjvankampen approved these changes

@gurkanindibay gurkanindibay Awaiting requested review from gurkanindibay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@darwing1210 @mjvankampen @gurkanindibay