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

pyserial: make serial.threaded.ReaderThread generic in the type of Protocol #13425

Merged
merged 6 commits into from
Jan 23, 2025

Conversation

x11x
Copy link
Contributor

@x11x x11x commented Jan 22, 2025

This change makes serial.threaded.ReaderThread generic in the type of the serial.threaded.Protocol that it wraps.

Currently, ReaderThread's connect and context-manager __enter__ methods, along with the protocol instance property, return a serial.threaded.Protocol. However, it is usually the case that ReaderThread will wrap a specific subclass of serial.threaded.Protocol, which has additional protocol-specific methods defined on it. So the return value of serial.threaded.Protocol is not really useful and it needs to be cast to the actual serial.threaded.Protocol subclass type, so that other protocol methods can be called without causing a type-checker error.

This change makes ReaderThread generic, and the wrapped serial.threaded.Protocol type a TypeVar, so that ReaderThread methods can return the specific serial.threaded.Protocol subclass rather than serial.threaded.Protocol itself.

This comment has been minimized.

…otocol

This change makes `serial.threaded.ReaderThread` generic in the type of the `serial.threaded.Protocol` that it wraps.

Currently, `ReaderThread`'s `connect` and context-manager `__enter__` methods, along with the `protocol` instance property, return a `serial.threaded.Protocol`. However, it is usually the case that `ReaderThread` will wrap a specific subclass of `serial.threaded.Protocol`, which has additional protocol-specific methods defined on it. So the return value of `serial.threaded.Protocol` is not really useful and it needs to be cast to the actual `serial.threaded.Protocol` subclass type, so that other protocol methods can be called without causing a type-checker error.

This change makes `ReaderThread` generic, and the wrapped `serial.threaded.Protocol` type a TypeVar, so that `ReaderThread` methods can return the specific `serial.threaded.Protocol` subclass rather than `serial.threaded.Protocol` itself.

This comment has been minimized.

@x11x
Copy link
Contributor Author

x11x commented Jan 22, 2025

I'm not sure if this will require users to change annotations using ReaderThread to ReaderThread[X] (explicitly specifying the Protocol type). If so that might be a breaking change?
I think in most cases, type checkers should be able to infer the type of the type variable (the Protocol subclass) from the return value of the protocol factory? so just ReaderThread should still work.

@x11x
Copy link
Contributor Author

x11x commented Jan 22, 2025

Here is a link to the implementation in case it helps anyone reviewing.

This comment has been minimized.

This comment has been minimized.

@x11x
Copy link
Contributor Author

x11x commented Jan 22, 2025

I'm not sure what would be the preferred way to deal with the F821 (undefined-name) linter error for default=Protocol -- I added a # noqa: F821 but now flake8 doesn't like that.

Should I quote "Protocol", or move the TypeVar below Protocol definition?

It seems like this should be fixed in Ruff so it is treated the same way as bound parameter -- unquoted forward references should be allowed here?

@x11x
Copy link
Contributor Author

x11x commented Jan 22, 2025

I have opened an issue in Ruff: astral-sh/ruff#15677

@x11x
Copy link
Contributor Author

x11x commented Jan 22, 2025

Also, I may not be understanding the linter config correctly but there is potentially an issue in the typeshed linter config

typeshed/pyproject.toml

Lines 36 to 38 in ce521e8

# We still use flake8-pyi and flake8-noqa to check these (see .flake8 config file);
# tell ruff not to flag these as e.g. "unused noqa comments"
external = ["F821", "NQA", "Y"]

select = NQA, Y, F821

it seems actually Ruff is checking the F821 as well.

I'm not sure if Ruff is actually checking unused-noqa (RUF100). The NQA102 "# noqa: F821" has no matching violations seems to be coming from flake8/flake8-noqa.

It looks like RUF100 in Ruff can replace NQA101, NQA102, and NQA103 from flake8-noqa. So we could possibly change linter config so that flake8 does not check NQA101, NQA102, and NQA103 but Ruff does check RUF100?

To fix Ruff undefined-name (F821)

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks! I'm not familiar with pyserial, but the changes as described by you make sense to me.

stubs/pyserial/serial/threaded/__init__.pyi Outdated Show resolved Hide resolved
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 3580566 into python:main Jan 23, 2025
43 checks passed
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