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 the error in generating the idle connection list #920

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zhanghanyun
Copy link

Fix the error in generating the idle connection list

@tomchristie
Copy link
Member

Thanks. There's a linting failure that needs resolving before we can merge this...

https://github.com/encode/httpcore/actions/runs/9440079124/job/25998938060?pr=920

./scripts/lint should auto-resolve this. I'm curious why we've not picked this up in the test suite. Worth taking a look at if we've any test cases for max_keepalive_connections perhaps?

@tomchristie
Copy link
Member

tomchristie commented Jun 10, 2024

We currently have a test case test_connection_pool_with_no_keepalive_connections_allowed which deals with the max_keepalive_connections=0 case.

Is it worth including test_connection_pool_with_one_keepalive_connection_allowed and test_connection_pool_with_two_keepalive_connections_allowed test cases as part of this PR? Would those test cases validate this change?

@tomchristie
Copy link
Member

Okay, let's just take this as-is... it's self-evidently an improvement.
If you could add an entry to the CHANGELOG and then let's get this merged.

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.

2 participants