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

chore: improve type hints #2867

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

Conversation

waketzheng
Copy link

@waketzheng waketzheng commented Feb 7, 2025

Summary

Only improve type hints, does not change any logic

  1. typing.xxx --> from typing import xxx
  2. Run ruff check --fix starlette tests
  3. from typing import ContextManager --> from contextlib import AbstractContextManager

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Feb 7, 2025

Can we enforce those via ruff?

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

If you can find ruff rules to enforce this behavior, I'm happy to accept it.

@waketzheng
Copy link
Author

waketzheng commented Feb 8, 2025

Run ruff check --select=UP --fix . can change from typing import Sequence to from collections.abc import Sequence, but it does not change the typing.Sequence. So I changed all typing.xxx to be from typing import xxx.

A mini demo to verify:

# bb.py
import typing
from typing import Callable, ContextManager, Sequence


def foo(a: Callable, b: ContextManager, c: Sequence) -> tuple[typing.Sequence, typing.ContextManager]:
    pass

Run ruff check bb.py will get the following output:

bb.py:2:1: UP035 [*] Import from `collections.abc` instead: `Sequence`
  |
1 | import typing
2 | from typing import Callable, ContextManager, Sequence
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP035
  |
  = help: Import from `collections.abc`

bb.py:2:1: UP035 `typing.ContextManager` is deprecated, use `contextlib.AbstractContextManager` instead
  |
1 | import typing
2 | from typing import Callable, ContextManager, Sequence
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ UP035
  |

Found 2 errors.
[*] 1 fixable with the `--fix` option.

Then run ruff check --fix bb.py, the file was updated to be:

import typing
from collections.abc import Sequence
from typing import Callable, ContextManager


def foo(a: Callable, b: ContextManager, c: Sequence) -> tuple[typing.Sequence, typing.ContextManager]:
    pass

@waketzheng waketzheng requested a review from Kludex February 11, 2025 03:14
@waketzheng
Copy link
Author

Can we enforce those via ruff?

Yes, ruff --select=UP . do that.

@Kludex
Copy link
Member

Kludex commented Feb 11, 2025

Can we enforce those via ruff?

Yes, ruff --select=UP . do that.

But you replaced everything in this PR manually... Right? The diff duplicated since last time I checked.

@waketzheng
Copy link
Author

Awaitable, Generator, Sequence, ... was auto moved to collections.abc by ruff check --fix .

While from typing import AsyncContextManager, ContextManager received complaint from ruff check and was replaced to from contextlib import AbstractAsyncContextManager, AbstractContextManager manually.

@waketzheng
Copy link
Author

I pushed two commits since you assigned it to me.
image

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