Skip to content

gh-132106: Allow logging.handlers.QueueListener to be used as a context manager #132107

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

Merged
merged 10 commits into from
Apr 12, 2025

Conversation

csm10495
Copy link
Contributor

@csm10495 csm10495 commented Apr 5, 2025

csm10495 and others added 3 commits April 5, 2025 08:32
Co-authored-by: Brian Schubert <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>
…e running (prevents a thread leak)

Update docs
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I'd prefer if we can make the multi-start check in a separate PR so that it becomes much easier to revert commits if needs arise. You can re-use the same issue however. We would then merge this PR afterwards (I'll make it so that we don't hang too much on this PR)

@csm10495
Copy link
Contributor Author

csm10495 commented Apr 6, 2025

@picnixz , I removed the listener already started stuff. I'll do a fresh PR after this one merges (since otherwise I'll merge conflict anyways with this one).. (Unless you'd like that one first instead?)

@picnixz
Copy link
Member

picnixz commented Apr 6, 2025

I removed the listener already started stuff. I'll do a fresh PR after this one merges (since otherwise I'll merge conflict anyways with this one).. (Unless you'd like that one first instead?)

Thank you! whichsoever order is fine.

@python-cla-bot
Copy link

python-cla-bot bot commented Apr 6, 2025

All commit authors signed the Contributor License Agreement.

CLA signed

@csm10495 csm10495 requested a review from vsajip April 6, 2025 16:53
@csm10495
Copy link
Contributor Author

@picnixz @vsajip mind taking another look? Thanks

Copy link
Member

@vsajip vsajip left a comment

Choose a reason for hiding this comment

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

I'd suggest removing the skipUnless ... QueueHandler decorators. Otherwise LGTM, thanks!

@bedevere-app
Copy link

bedevere-app bot commented Apr 11, 2025

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@csm10495 csm10495 requested a review from vsajip April 11, 2025 05:18
@csm10495
Copy link
Contributor Author

I have made the requested changes; please review again

@bedevere-app
Copy link

bedevere-app bot commented Apr 11, 2025

Thanks for making the requested changes!

@vsajip: please review the changes made to this pull request.

@picnixz picnixz enabled auto-merge (squash) April 12, 2025 11:37
@picnixz picnixz merged commit 517e96b into python:main Apr 12, 2025
39 checks passed
@picnixz
Copy link
Member

picnixz commented Apr 12, 2025

Thanks for the work. You can now fixup the multiple start issue in a new PR but with the same issue number.

@csm10495
Copy link
Contributor Author

Thanks for the work. You can now fixup the multiple start issue in a new PR but with the same issue number.

#132444

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.

4 participants