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

Fix Local behaviour with asyncio Task. #478

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

spanezz
Copy link
Contributor

@spanezz spanezz commented Oct 6, 2024

Reproduce issue #473 in asgiref's test suite

Fixes #473

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

@spanezz That looks like a good port of the test 👍

@carltongibson carltongibson changed the title Reproduce issue #473 Fix Local behaviour with asyncio Task. #473 Oct 7, 2024
@carltongibson
Copy link
Member

Thanks @spanezz 👍

The type hints need adjusting to keep mypy happy. (See CI)

I had a glance at the different commits, but will try and reason them through this week.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

@spanezz I had a quick look. Both of these make sense. Not sure if I prefer one or the other. Option 1 is less of a change.

Can you fix the CI, and perhaps squash the last two commits together so that option 1 and option 2 are obvious for @andrewgodwin to look at?

@spanezz
Copy link
Contributor Author

spanezz commented Oct 8, 2024

@spanezz I had a quick look. Both of these make sense. Not sure if I prefer one or the other. Option 1 is less of a change.

Can you fix the CI, and perhaps squash the last two commits together so that option 1 and option 2 are obvious for @andrewgodwin to look at?

I made an attempt. I don't seem to be able to run the CI myself, so hopefully it won't take too many iterations to get right.

I'd defer fixing things like Local being changed from a class to a function, until it's decided if that's what's wanted

@carltongibson carltongibson changed the title Fix Local behaviour with asyncio Task. #473 Fix Local behaviour with asyncio Task. Oct 9, 2024
@carltongibson
Copy link
Member

carltongibson commented Oct 9, 2024

@spanezz You can run the type checker locally by doing (e.g.) tox -e py311-mypy.

OK, everything passes with 16d4ae2 Option 1. Can I ask you to pull 7b3ce47 Option 2 into a separate PR (after this one, I guess).

I think we should go with Option 1 now, and release a v3.8.2 to fix the issue, and then we can consider whether Option 2 is worth adopting for a v3.9 at a more leisurely pace. //cc @andrewgodwin

I closed #477 which had the self._data.set({**self._data.get({}), key: value}) option.

@spanezz
Copy link
Contributor Author

spanezz commented Oct 9, 2024

@spanezz You can run the type checker locally by doing (e.g.) tox -e py311-mypy.

Thank you for the pointer! I am uncomfortable running code randomly downloaded off the internet on my devel machine, and I'm unfortunately not equipped with a suitably containerized alternative.

OK, everything passes with 16d4ae2 Option 1. Can I ask you to pull 7b3ce47 Option 2 into a separate PR (after this one, I guess).

I think we should go with Option 1 now, and release a v3.8.2 to fix the issue, and then we can consider whether Option 2 is worth adopting for a v3.9 at a more leisurely pace. //cc @andrewgodwin

I closed #477 which had the self._data.set({**self._data.get({}), key: value}) option.

Done (I removed the simplification commit from this PR and opened #481)

@andrewgodwin
Copy link
Member

This definitely seems like a pretty clean change and easier to understand than the bigger rewrite, so let's get this in.

I removed the python 3.8 tests from main - if you want to rebase on top of that so we get a clean sweep, I'm happy to merge this.

@spanezz
Copy link
Contributor Author

spanezz commented Oct 10, 2024

This definitely seems like a pretty clean change and easier to understand than the bigger rewrite, so let's get this in.

I removed the python 3.8 tests from main - if you want to rebase on top of that so we get a clean sweep, I'm happy to merge this.

Thanks, and done!

@andrewgodwin andrewgodwin merged commit 85d2445 into django:main Oct 11, 2024
6 checks passed
@andrewgodwin
Copy link
Member

Thanks!

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.

asgiref.local.Local fails to isolate changes between asyncio tasks
3 participants