-
Notifications
You must be signed in to change notification settings - Fork 67
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
⬆️ UPGRADE: Drop support for EOL Python 3.6 #194
Conversation
Codecov Report
@@ Coverage Diff @@
## master #194 +/- ##
==========================================
- Coverage 96.17% 96.14% -0.04%
==========================================
Files 61 61
Lines 3270 3244 -26
==========================================
- Hits 3145 3119 -26
Misses 125 125
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I added an "all good" CI job. Currently we have "tests (3.6)" and "tests (pypy-3.6)" set as Required jobs in GitHub but they no longer exist in the CI matrix. It can be annoying to update the set of required jobs whenever dropping or adding python versions so this is a workaround: we only require the "allgood" job to pass, and the "allgood" job only passes if all existing test jobs and pre-commit job passes. I can remove this if it's not something you like. |
Cheers, I'll probably have a look at these next week now |
@@ -15,7 +17,7 @@ | |||
version_str = "markdown-it-py [version {}]".format(__version__) | |||
|
|||
|
|||
def main(args: Optional[Sequence[str]] = None) -> int: | |||
def main(args: Sequence[str] | None = None) -> int: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I didn't think type unions were supported yet: https://www.python.org/dev/peps/pep-0604/
But I guess if it works it works? Or is there any issue with type evaluation pre python 3.10?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dammit, made this comment ages ago, but didn't realise it was pending
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works perfectly!
It would be a syntax error (pre Python 3.10) but from __future__ import annotations
(first available in Python 3.7) makes it not error.
And mypy
doesn't care what our minimum supported Python version is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how mypy does that 🤷 (are you sure it actually picks them up, and not silently fails)
On python 3.10 you can do:
In [1]: def bar() -> int | str:
...: pass
In [2]: from typing import get_type_hints
In [3]: get_type_hints(bar)
Out[3]: {'return': int | str}
but on python 3.8, you get:
In [1]: from __future__ import annotations
In [2]: from typing import get_type_hints
In [3]: def bar() -> int | str:
...: pass
...:
In [4]: get_type_hints(bar)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-4-348bd5169782> in <module>
----> 1 get_type_hints(bar)
~/Documents/GitHub/aiida_core_develop/.tox/py38-pre-commit/lib/python3.8/typing.py in get_type_hints(obj, globalns, localns)
1262 if isinstance(value, str):
1263 value = ForwardRef(value)
-> 1264 value = _eval_type(value, globalns, localns)
1265 if name in defaults and defaults[name] is None:
1266 value = Optional[value]
~/Documents/GitHub/aiida_core_develop/.tox/py38-pre-commit/lib/python3.8/typing.py in _eval_type(t, globalns, localns)
268 """
269 if isinstance(t, ForwardRef):
--> 270 return t._evaluate(globalns, localns)
271 if isinstance(t, _GenericAlias):
272 ev_args = tuple(_eval_type(a, globalns, localns) for a in t.__args__)
~/Documents/GitHub/aiida_core_develop/.tox/py38-pre-commit/lib/python3.8/typing.py in _evaluate(self, globalns, localns)
516 localns = globalns
517 self.__forward_value__ = _type_check(
--> 518 eval(self.__forward_code__, globalns, localns),
519 "Forward references must evaluate to types.",
520 is_argument=self.__forward_is_argument__)
<string> in <module>
TypeError: unsupported operand type(s) for |: 'type' and 'type'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it works. Mypy is a static type checker. It doesn't run the code it type checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep: https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-generic-builtins/https://mypy.readthedocs.io/en/stable/runtime_troubles.html#using-x-y-syntax-for-unions
I doubt that anyone will need to dynamically evaluate these types, but something to bear in mind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't run the code it type checks
Not sure about sphinx autodoc though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah think it just uses:
In [13]: import inspect
In [16]: def bar() -> int | str:
...: pass
...:
In [17]: inspect.signature(bar).return_annotation
Out[17]: 'int | str'
and never evaluates, so should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought sphinx-autodoc doesn't do anything with type annotations unless sphinx-autodoc-typehints is in use (it isn't)?
And the plugin certainly supports what we do in this PR: tox-dev/sphinx-autodoc-typehints#184
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeh exactly, looks all good 👍; I just wanted to make sure I understood the implications of using future annotations, before rolling them out everywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers!
Closes #193