Add type-hinting to concurrent decorators. Bump to Python 3.9. #139
Conversation
|
Big fan of this PR. However I'm against the bump to 3.9. This is a library after all, the core functionality is not affected and so bumping will only harm users stuck on older python versions. There is a way out of this, you could either split the types into a stub file or use typing_extensions like so: from typing import TYPE_CHECKING:
if TYPE_CHECKING:
# look ma. No runtime errors on older python!
from typing_extensions import ...etc |
|
|
||
| def process(*args, **kwargs) -> Callable: | ||
| @overload | ||
| def process(func: Callable[[_P], _T]) -> Callable[[_P], common.ProcessFuture[_T]]: ... |
There was a problem hiding this comment.
I'd recommend using the typing.ParamSpec here
|
Understood; I don't have experience writing type-safe code before 3.9 so I'm less familiar with how to write proper type hints before the changes introduced in that version. I'm at a conference right now but I will try to dive in after this week and test against 3.7. |
I'm not a maintainer of this package. But I'd you'd like I can help out and make a PR targeting your branch. |
|
Hi @emiliadecaudin, thanks for your contribution! I second @JoaquimEsteves feedback, although Python 3.8 is going EOL in October, it would be great to still support older versions as this module is used by quite few projects. This does not mean we have to backport this feature in previous versions. We simply need to support the case in which an older Python interpreter is used. As type hints are a non functional feature, we can definitely skip support for older Python versions. Just make this functionality optional. 😉 |
| @@ -26,8 +26,9 @@ def __init__(self, msg, code=0): | |||
| super(ProcessExpired, self).__init__(msg) | |||
| self.exitcode = code | |||
|
|
|||
| _T = TypeVar("_T") | |||
There was a problem hiding this comment.
Here you could do:
try:
from typing import TypeVar
_T = TypeVar("_T")
except ImportError:
_T = "GenericType"Have not tested it but it might be enough of a change to support old interpreters.
| from concurrent.futures import CancelledError, TimeoutError | ||
|
|
||
| from pebble import common | ||
|
|
||
| _P = TypeVar("_P") |
There was a problem hiding this comment.
Same as here.
As this logic is common for both process and thread decorators, I would place it into pebble.common.types and use it around so we don't clutter the whole code base with backwards compatible supporting logic (try/excepts etc...).
| @@ -24,7 +24,7 @@ def package_version(): | |||
| url="https://github.com/noxdafox/pebble", | |||
| packages=find_packages(exclude=["test"]), | |||
| long_description=open(os.path.join(CWD, 'README.rst')).read(), | |||
| python_requires=">=3.6", | |||
| python_requires=">=3.9", | |||
There was a problem hiding this comment.
As said, let's try to support 3.6 until possible. Type hinting is not a functional feature hence it does not justify dropping backwards compatibility.
Keep in mind that we use semver here. Hence, by doing the above, we should bump Pebble version to 6.0.0. Not sure it's the right choice. 😄
I've confirmed before submitting this pull request that all tests pass.
This pull request does two things:
threadandprocessdecorators in theconcurrentsub-package. This allows users of this package to properly treat the output of a decorated function as aFutureorProcessFuture, respectively, instead of the type-checker treating the function signature as identical to the original function.I am a big fan of this package and would be eager to help add more type-hinting for the rest of the package as well as adding mypy/ruff linting!