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

Move variable initialization in AsyncToSync from __init__ to __call__ #439

Closed
wants to merge 2 commits into from

Conversation

germaniuss
Copy link
Contributor

Closes #438

@andrewgodwin
Copy link
Member

OK, so the code still looks good, but since this code is best tested to within an inch of its life - think you can write a test that ensures this behaviour is there in future?

@germaniuss
Copy link
Contributor Author

OK, so the code still looks good, but since this code is best tested to within an inch of its life - think you can write a test that ensures this behaviour is there in future?

Sure thing

@germaniuss
Copy link
Contributor Author

germaniuss commented Jan 22, 2024

Done! Wasn't really sure about the test naming though.

@andrewgodwin
Copy link
Member

andrewgodwin commented Jan 27, 2024

I think the test naming is fine - just satisfy the linter, and I'll land it!

EDIT: Wait, 3.8 failed, womp womp. It's still in security release mode so we do still need to support it.

@ttys0dev
Copy link
Contributor

ttys0dev commented Jan 30, 2024

Wait, 3.8 failed, womp womp.

Well we can just conditionally disable that specific test that's breaking on 3.8 like in #440.

@andrewgodwin
Copy link
Member

Merged in the form of #440 instead.

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.

async_to_sync does not use the correct thread when used as decorator
3 participants