-
Notifications
You must be signed in to change notification settings - Fork 120
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
Drop support for old python versions and add type annotations #492
Conversation
This pull request introduces 2 alerts when merging 7e89d6a into 581cba4 - view on LGTM.com new alerts:
|
This pull request introduces 2 alerts when merging fdc5c56 into 581cba4 - view on LGTM.com new alerts:
|
In general I agree with this, but I would suggest for this PR to only drop obsolete python version support, and adding annotations to existing methods where we find that useful. We should consider adding more wrapper methods (and to what detail we want to specify those) in a separate follow-up PR. |
Sounds good to me! I'll force push this PR to remove the extra wrapper methods |
This pull request introduces 2 alerts when merging 77b0616 into 581cba4 - view on LGTM.com new alerts:
|
docs/installation.rst
Outdated
@@ -1,25 +1,18 @@ | |||
Installation | |||
============ | |||
|
|||
The Neovim Python client supports Python 2.7, and 3.4 or later. | |||
The Neovim Python client supports Python 3.6 or later. |
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.
3.6 is end-of-life now, too (since Dec 23, 2021), so might as well bump to 3.7 (which still has till June 27, 2023).
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.
Bumped. Note that I left the python_requires=">=3.6"
in the setup.py because of CI issues. The latest pypy3 in Ubuntu repos is only compatible with Python 3.6, so if we want to bump the python_requires
we have to either stop testing on pypy3 (probably undesired) or wrangle the CI environment to install a more recent pypy3. I think that's outside of the scope of this PR.
We probably want to move to github workflows anyway, since travis-ci got rid of their easy-to-use free tier for open source projects.
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.
Yeah, looks like travis is already broken because it depends on https://github.com/neovim/bot-ci, which was archived.
This pull request introduces 2 alerts when merging b17943a into 71102c0 - view on LGTM.com new alerts:
|
This PR is awesome! Any way this can be reviewed/merged? |
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 is an amazing PR, I've left some comments at a couple of lines.
One thing: I see a lot of var: SomeType = None
instead of var: Optional[SomeType] = None
overall in many places. Why not using Optional
?
@@ -45,6 +40,7 @@ | |||
packages=['pynvim', 'pynvim.api', 'pynvim.msgpack_rpc', | |||
'pynvim.msgpack_rpc.event_loop', 'pynvim.plugin', | |||
'neovim', 'neovim.api'], | |||
python_requires=">=3.6", |
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 should be >=3.7
as well
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.
As mentioned in the comment above, this is because bumping the python_requires
breaks the CI.
def attach( | ||
session_type: TTransportType, | ||
address: str = None, | ||
port: int = 7450, |
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.
Is having a default argument here OK? Also, str = None
is not a valid typing annotation. I think they might be port: Optional[int] = None
or address: Optional[str] = None
, etc.
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.
You are correct that the explicit Optional[type]
is now the preferred style, though it was not always such, and mypy I think still supports implicitly inferring the Optional
(it did at the time of writing this PR, but it's been a while).
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.
The default argument does seem like a little bit of a weird change to be included in this PR... maybe something that was left in from testing/debugging?
Any update on getting this merged? |
Python 3.5 reached end-of-life September 5, 2020 (https://www.python.org/downloads/release/python-3510/). 3.6 is the first version with support for the type annotation syntax (https://www.python.org/dev/peps/pep-0526/)
It was an exact copy brought in for python 2 compatibility
This adds coverage for most of the public API, but there are still some areas that aren't covered yet. * script_host.py is ignored entirely * host.py is ignored entirely * the msgpack_rpc submodule has partial coverage * there are some Any annotations sprinkled around on some of the more complex functions Funtionality should remain largely unchanged. Notable exceptions are: * Buffer.mark() and Window.cursor now return a tuple instead of a list. This is because there is currently no type for a fixed-size list (python/mypy#7509)
Rebased. #526 resurrected CI but Windows tests are currently broken. That's unrelated to this PR so doesn't block this PR. |
Fixes #422
Dropping support for python 3.5 and earlier (3.5 reached end-of-life September 5, 2020)
This adds coverage for most of the public API, but there are still some
areas that aren't covered yet.
script_host.py
is ignored entirelyhost.py
is ignored entirelymsgpack_rpc
submodule has partial coveragecomplex functions
I recommend reviewing the commits in order