Refine types for distutils.version #1835
Conversation
Note: these type signatures were derived mainly by looking at the docstrings inside distutils.version. For reference: - py3 impl: https://github.com/python/cpython/blob/master/Lib/distutils/version.py - py2 impl: https://github.com/python/cpython/blob/2.7/Lib/distutils/version.py
stdlib/2and3/distutils/version.pyi
Outdated
| def __ge__(self: T, other: Union[T, str]) -> bool: ... | ||
|
|
||
| # Methods part of the 'Version' interface, but not implemented | ||
| # within the class itself |
There was a problem hiding this comment.
Isn't the recommended pattern here to use @AbstractMethod in the base class and add concrete methods to the subclasses? (Or, as a compromise, make only __init__ abstract and add just a concrete __init__ to the subclasses.) Since presumably you can't instantiate Version.
There was a problem hiding this comment.
That's true -- the main reason I didn't do that is because the actual implementations of version.py don't import and use the abc module or use the @abstractmethod decorator.
Should I go ahead and do that within the stubs? I don't quite remember if there are any issues with having just the stubs use AbcMeta as a metaclass when the concrete implementations don't.
|
At least for mypy, you don't even need to use ABCMeta -- unless CPython's runtime, mypy checks |
|
Ok, I made those changes. I made (That said, I think decorating |
stdlib/2and3/distutils/version.pyi
Outdated
|
|
||
| @abstractmethod | ||
| def __init__(self, vstring: Optional[str] = None) -> None: ... |
There was a problem hiding this comment.
Defaults should always be ....
stdlib/2and3/distutils/version.pyi
Outdated
|
|
||
| def __init__(self, vstring: Optional[str] = None) -> None: ... | ||
| def parse(self: _T, vstring: str) -> _T: ... | ||
| def __str__(self) -> str: ... |
There was a problem hiding this comment.
I don't think this needs to be in the stub, since it's unchanged from the base class.
There was a problem hiding this comment.
I think we do need them -- without them, mypy complains that it can't instantiate StrictVersion and LooseVersion because they contain abstract attributes.
There was a problem hiding this comment.
Oh, because the base class has an abstract __str__ implementation; I didn't notice that.
|
@JelleZijlstra -- I think I fixed the things you pointed out in your review. Was there anything else you wanted me to change? |
* python/master: (269 commits) allow Optional[float] for asyncio.wait() timeout arg (python#1860) Clean up the pytype blacklist. (python#1851) filter function: make the callable of the first overload non-optional so that mypy is able to select the second overload for the case. (python#1855) Accept Text in distutils *Version classes (python#1854) Add attrs library (python#1838) Add type stub for the lzma module (python#1844) Add ImportError attributes name, path for Python 3.3+ (python#1849) Add 'message' to click.decorators.version_option (python#1845) PEP 553 and PEP 564 (python#1846) Adding py36 additions to datetime module (python#1848) Remove incomplete thrift stubs that cause false positives. (python#1827) adding curses.ascii, curses.panel and curses.textpad modules (python#1792) Fix requests session hooks type (python#1794) Add StringIO.name and BytesIO.name (python#1802) Fix werkzeug environ type (python#1831) Change the return type of unittest.TestCase.fail() to NoReturn (python#1843) _curses.tparm works on bytes, not str (python#1828) Refine types for distutils.version (python#1835) Add stub for stat.filemode (python#1837) modify pytype_test to run from within pytype too, and support python3 (python#1817) ...
Note: these type signatures were derived mainly by looking at the docstrings inside distutils.version. For reference:
(Context for why I'm improving this (obscure) corner of stdlib: I'm running mypy with nearly every warning enabled on a project, and it complained that this module was missing type signatures.)