-
Notifications
You must be signed in to change notification settings - Fork 118
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
Fix 'SyntaxError: invalid syntax' on Python 3.7 alpha pre-release #274
Conversation
neovim/api/buffer.py
Outdated
return self.request('nvim_buf_add_highlight', src_id, hl_group, | ||
line, col_start, col_end, async=async) | ||
line, col_start, col_end, _async=_async) |
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.
Does the parameter really need to be changed? That's silly, python parser should know the difference.
This will be a breaking change for our users
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 machinery replaced all async
into _async
so I'm not sure about this particular line.
However the screenshot I pasted showed invalid syntax
on function attributes and this PR fixed that issue.
I guess we need to detect the python version and show a warning. |
While Python 3.7 is in alpha stage yet, this PR may not required in future. |
So let's wait until python 3.7 is at least in beta. Hopefully they will gather their senses and make it a context-dependent keyword (the pattern used by python in the past). If not, we must keep the existing |
Thanks for the PR @lambdalisue. FYI python 3.7 is now in beta. |
Is the breaking change still there? I looked into https://docs.python.org/3.7/whatsnew/3.7.html and grepped for both "async" and "keyword", and found nothing. |
@bfredl https://docs.python.org/3.6/whatsnew/3.6.html#new-keywords It is added in
Note: It should be added in |
Looks like other popular projects deal with this in the same way. Though |
Python 3.7.0b2 still has this issue. |
Shall I replace |
@lambdalisue yes, thanks. |
3b156e7
to
6d1b28b
Compare
Done. It seems tests are failing on Additionally, you said
So I added |
neovim/msgpack_rpc/session.py
Outdated
warnings.warn( | ||
'"async" attribute is deprecated. Use "async_" instead.', | ||
DeprecationWarning, | ||
) |
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 looks like a good solution to me.
If no one else comments, ping in a few days, then we merge.
Looks like AppVeyor is drunk. |
I broke master when I merged #290 , that's why travis is failing. Fixed by neovim/neovim#8265. Restarted travis build. |
@justinmk Ping |
neovim/api/buffer.py
Outdated
"""Add a highlight to the buffer.""" | ||
if async is None: | ||
async = (src_id != 0) | ||
if async_ is None and 'async' in kwargs: |
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 seems like it can be a helper function:
async_ = check_async(async_, kwargs, (src_id != 0))
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.
Should be? It's just one line and used only here so I don't think it's a good idea to extract a simple code into a helper function.
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.
How so? it is 8-9 lines unrelated to the original meaning of the function in at least three places (here, clear_highlight
, and request
)
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 got. Sorry, I thought you are talking about exactly this line.
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.
Fixed 👍
neovim/msgpack_rpc/event_loop/uv.py
Outdated
@@ -13,7 +13,7 @@ class UvEventLoop(BaseEventLoop): | |||
|
|||
def _init(self): | |||
self._loop = pyuv.Loop() | |||
self._async = pyuv.Async(self._loop, self._on_async) |
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.
invalid substitution.
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.
👍
Other than comments, LGTM. |
neovim/msgpack_rpc/event_loop/uv.py
Outdated
@@ -13,7 +13,7 @@ class UvEventLoop(BaseEventLoop): | |||
|
|||
def _init(self): | |||
self._loop = pyuv.Loop() | |||
self._async = pyuv.Async(self._loop, self._on_async) | |||
self.async_ = pyuv.Async(self._loop, self._on_async) |
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 the internal variable name, it should not change.
Always check changes after search-and-replace ...
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 README.md (and any other docs) needs to be updated, too.
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.
Always check changes after search-and-replace ...
I'm sorry for that. I didn't realize that there is a private variable for async
.
I'll recheck.
The README.md (and any other docs) needs to be updated, too.
Shall I? I'm not native so writing docs is a bit hard for me.
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.
Done by a9a3a70 Is it enough?
'async' is keyword in Python 3.7 https://www.python.org/dev/peps/pep-0492/#deprecation-plans
This commit should be reverted sometime in the future.
Done. |
What release is this planned to be included in? |
Unrelated failure (I presume):
Merged. Thanks @lambdalisue for you work. |
@jdemilledt 3.7 final is due to 15 june, we will have a release well before then. If I get around to it I will finish the lua PR #325 next week, and then do a release. |
This release adds support for Python 3.7 which now is in beta. `async` is a keyword in python3.7, and it should be replaced with `async_` when used as a keyword argument to API methods. `async` is still supported with python 2.7 and 3.6 for the moment, but considered deprecated. Also, integration with the in-process lua interpreter in Nvim is now supported. Changes since 0.2.4: * debcde0 clean up remote object implementation (#313) * e880fe7 Guidelines for local plugin development (#317) * 1ab98e8 Update the working directory on DirChanged for legacy plugins (#296) * d53415d Fix SyntaxError in py3.7: use `async_` instead of `async` (#274) * b65f62d Use `pytest` as `nosetests` are not longer maintained (#266) * d9aed96 Support using lua functions. `buf.update_higlights` as an internal example (#325)
This release adds support for Python 3.7. Previously we didn't allow that version because python-client had problems with it, since arguments were named after keywords newly introduced in 3.7. Fixed since of neovim/pynvim#274
Reason
'async' is keyword in Python 3.7
https://www.python.org/dev/peps/pep-0492/#deprecation-plans