Skip to content

improve typehinting belonging to function _wait_one - #261

Open
Vizonex wants to merge 4 commits into
aio-libs:mainfrom
Vizonex:improved-typing
Open

Vizonex wants to merge 4 commits into
aio-libs:mainfrom
Vizonex:improved-typing

Conversation

@Vizonex

@Vizonex Vizonex commented Sep 25, 2026

Copy link
Copy Markdown
Member

What do these changes do?

These changes alter the type hinting behavior of _wait_one to lessen the confusion as to what exactly the function is doing which is picking an awaitable container to proceed with. A Future[_T] is then returned rather than just an ordinary object. (Hence my decision of removing Any all together from the futures argument) return value is then set to asyncio.Future[_T] which is the correct object being returned.

Are there changes in behavior for the user?

These changes are all internal and only affect the maintainers of this project. My goal of this PR was to lessen the confusion for newer maintainers or users who wish to know what is going on in the code itself. The original type hints were confusing but at the very least correctable.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes

@codspeed

codspeed Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 14 untouched benchmarks


Comparing Vizonex:improved-typing (66360b7) with main (d3ba49e)

Open in CodSpeed

@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (d3ba49e) to head (66360b7).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #261   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          226       226           
  Branches        43        43           
=========================================
  Hits           226       226           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greptile-apps

greptile-apps Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

[Medium risk] Refines type hints in an internal async utility function.

No outstanding findings block merging.

Reviews (3) · Last reviewed commit: "Update src/aiohappyeyeballs/_staggered.p..."

Comment thread src/aiohappyeyeballs/_staggered.py
@greptile-apps

This comment has been minimized.

@aiolibsbot

Copy link
Copy Markdown
Contributor

PR Review — improve typehiting belonging to function _wait_one

Correct diagnosis of a real typing hole, but the chosen signature breaks the only call site and turns the mypy lint job red.

What's solid: you spotted something genuinely wrong that had been sitting in the file — -> _T with _T appearing only in the return position let mypy infer the return type from assignment context, so the annotation asserted nothing and quietly permitted any call-site expectation. Annotating wait_next as Future[Future[_T]] and _on_completion's fut param makes the body internally consistent for the first time, and await wait_next now genuinely lines up with the declared return. The change is annotation-only, so runtime behavior and 100% coverage are untouched — consistent with the green Codecov report.

  • 🟡 futures: Iterable[asyncio.Future[_T]] is unsolvable at _staggered.py:154-156, where staggered_race passes Task[tuple[_T, int] | None] alongside Future[None]; asyncio.Future is invariant, so mypy errors there plus at tasks.remove(done) and done.result(). The lint job runs pre-commit including the mypy hook, so this is a red required check. Confirms @greptile-apps[bot]'s P1.
  • 🟡 Suggested reframing: a Future-bound TypeVar (_FutureT = TypeVar("_FutureT", bound="asyncio.Future[Any]"), Iterable[_FutureT] -> _FutureT) states the actual contract — "returns one of the futures you passed in" — and preserves call-site precision, which reusing the result-typed module-level _T cannot.
  • 🟢 Replace the # type: comment on line 25 with a PEP 526 annotation; the project floor is 3.10 and the rest of the module annotates inline.
  • ℹ️ The PR checklist claims unit tests and documentation reflect the changes; neither is in the diff. That's fine for an annotation-only change, but the boxes overstate it.
  • Note: I could not run mypy in this read-only review shell, so the failure is established by type-level reasoning over the real call site plus greptile's base-vs-PR artifacts, not by my own execution.

🟢 Suggestions

1. Prefer a PEP 526 annotation over a `# type:` comment
src/aiohappyeyeballs/_staggered.py:25

The nested future type is communicated via a legacy type comment:

wait_next = loop.create_future()  # type: asyncio.Future[asyncio.Future[_T]]

The project floor is Python 3.10 (requires-python = ">=3.10"), and every other annotation in this module uses inline syntax, so a variable annotation reads more consistently and is less easy to silently detach from its statement during future edits:

wait_next: "asyncio.Future[asyncio.Future[_T]]" = loop.create_future()

Since the PR's stated goal is reducing confusion for new readers, the inline form serves that goal better than a trailing comment.

wait_next = loop.create_future()  # type: asyncio.Future[asyncio.Future[_T]]

Checklist

  • Runtime behavior unchanged (annotation-only diff)
  • Public API unchanged / backward compatible
  • Annotation style consistent with module and 3.10+ floor — suggestion #1
  • Test coverage maintained (100%, no behavior to test)
  • No security-relevant surface touched
  • Scope matches PR description (internal typing only, no creep)

Automated review by Kōan (Claude) HEAD=6d8b33e 6 min 44s

@Dreamsorcerer Dreamsorcerer changed the title improve typehiting belonging to function _wait_one improve typehinting belonging to function _wait_one Sep 26, 2026
Comment thread src/aiohappyeyeballs/_staggered.py Outdated
if coro_fn := next(coro_iter, None):
this_index += 1
exceptions.append(None)
start_next = loop.create_future()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm guessing this needs an annotation or something.

Co-authored-by: Sam Bull <aa6bs0@sambull.org>
Comment thread src/aiohappyeyeballs/_staggered.py Outdated
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>

This branch has not been deployed

No deployments
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.

3 participants