-
Notifications
You must be signed in to change notification settings - Fork 197
Use asyncio #2880
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
base: main
Are you sure you want to change the base?
Use asyncio #2880
Changes from 91 commits
699dad3
908c849
b4f0420
76f3c3f
45d7ea6
e8fae30
c511cec
7438d4a
6a104b0
d59bb1c
c0bea09
c1fc59c
e35b611
6cad098
1597782
cc642f4
cea0e99
3d30ad4
472705b
ea34714
069ed50
776749a
0227877
2649a6f
54af917
1a4589c
900f721
35621cf
dbf4d7d
f3b2f84
a9c1692
7004df5
a876949
d27aa93
341ff99
4ed4e3f
7aba65e
e3ccba2
c2c2a8c
af561a8
7260c90
a60ddd2
23210e3
73ed644
6d6e772
a84f3dd
f7702c2
bb51929
7d58862
e2119a1
ce27231
a4aba53
c215234
ae081e3
e4d3324
6e5ca85
4866feb
73a658a
8a6de63
fcb8717
8997d51
5943571
7485b90
edd3ccd
91af0e0
da1fc1d
34697be
aa37898
9335c7d
afde9fb
0795ee4
4659e8d
76aeba1
66bd43a
a143bdf
69fd44d
c2a9e30
8835214
668846c
c6a89b1
9d8c1b3
d271624
68b8945
a3c88ec
3915ca6
82973a5
1ae40fe
45bbb05
998ac1c
9c761d1
056359b
aca3b34
1a6448b
dd2b791
ff9cfd7
5e3af9c
d91eeda
8676dc4
c6efba4
2c2e7b3
119fa99
9e35cc4
8c83c96
691d8ea
0aa553b
e9d65dd
a8f6ba1
a5d67f5
09b3d3d
bc70dee
7005ac0
750b995
e3fc6e9
6817036
3c90ff9
6e7f252
a5e3a6d
135aa60
9506bd5
0e19738
45ee4dd
ca61cc6
bb41f71
1529494
1f14cee
baec759
81e3ab8
89d8ae9
ff28a4e
0ab6df7
4801cd6
c7af523
f3d0684
9d2283d
fbfba1c
e39ccf2
4081b04
42111fb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ | |
| "bracex", | ||
| "mdpopups", | ||
| "orjson", | ||
| "sublime_aio", | ||
| "typing_extensions", | ||
| "wcmatch" | ||
| ] | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,8 @@ | |||||
| from ..protocol import CodeActionParams | ||||||
| from ..protocol import Command | ||||||
| from ..protocol import Diagnostic | ||||||
| from .core.aio import call_soon_threadsafe | ||||||
| from .core.aio import run_coroutine_threadsafe | ||||||
| from .core.promise import Promise | ||||||
| from .core.protocol import Error | ||||||
| from .core.protocol import Request | ||||||
|
|
@@ -65,9 +67,9 @@ def is_quickfix(action: Command | CodeAction) -> bool: | |||||
|
|
||||||
|
|
||||||
| def filter_quickfix_actions( | ||||||
| only_with_diagnostics: bool, response: list[Command | CodeAction] | Error | None | ||||||
| only_with_diagnostics: bool, response: list[Command | CodeAction] | BaseException | None | ||||||
|
Comment on lines
-68
to
+70
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting that we don't receive Does it make a difference in practice? Probably not if you've managed to convert the code without using
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what you mean here. The underlying reason why some of these response / error handlers now take a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also one can't just use I'm not saying that all futures should return
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes... one can Once again the reason why
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm saying that in places like Line 1041 in 1f14cee
Error because it's one of the possible return types that server can trigger.
With the old code (even though it used promises) it did work like that. The new code doesn't include EDIT: Actually, don't think it should be both. It should be
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception flow is something inherent to the I made JSON-RPC errors be part of this exception flow, which you are now questioning whether is suitable. I do believe it’s “natural” to let this be part of the regular exception flow (which is why I wrote it like this). A consequence of this is that one does have to write a bunch of try-except blocks in places where this wasn’t the case before. I think this enhances understanding of what may go wrong.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that's one of the points of review - to question things. Maybe the approach to treat JSON-RPC errors as exceptions isn't really correct? Disclaimer: I haven't even started properly reviewing the code yet. I'm just picking on things that stick out. So I don't have the full picture yet. But I'm really not sure if handling LSP
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't really followed and also haven't reviewed this PR yet, but there should be 1 or 2 places in the current code (on main) where we have special handling for certain error responses (iirc for pull diagnostics to retrigger requests when server is busy), and we might want to add some more handling of error responses for more requests in the future. There is also a Line 354 in 44f568a
|
||||||
| ) -> list[Command | CodeAction]: | ||||||
| if isinstance(response, Error) or not response: | ||||||
| if isinstance(response, BaseException) or not response: | ||||||
| return [] | ||||||
| if only_with_diagnostics: | ||||||
| # If there are multiple diagnostics for the region, in the hover popup we can only use those code actions which | ||||||
|
|
@@ -302,7 +304,7 @@ def _handle_response_async( | |||||
| if self._cancelled: | ||||||
| return | ||||||
| view = self._task_runner.view | ||||||
| tasks: list[Promise[None]] = [] | ||||||
| tasks: list[Promise[BaseException | None]] = [] | ||||||
| config_name, code_actions = response | ||||||
| session = self._task_runner.session_by_name(config_name, 'codeActionProvider') | ||||||
| if session and code_actions: | ||||||
|
|
@@ -386,9 +388,9 @@ def run( | |||||
| if code_actions_by_config: | ||||||
| self._handle_code_actions(code_actions_by_config, run_first=True) | ||||||
| return | ||||||
| self._run_async(only_kinds) | ||||||
| run_coroutine_threadsafe(self._run(only_kinds)) | ||||||
|
|
||||||
| def _run_async(self, only_kinds: list[str | CodeActionKind] | None = None) -> None: | ||||||
| async def _run(self, only_kinds: list[str | CodeActionKind] | None = None) -> None: | ||||||
| view = self.view | ||||||
| region = first_selection_region(view) | ||||||
| if region is None: | ||||||
|
|
@@ -427,13 +429,13 @@ def _handle_select(self, index: int, actions: list[tuple[ConfigName, CodeActionO | |||||
| if index == -1: | ||||||
| return | ||||||
|
|
||||||
| def run_async() -> None: | ||||||
| async def run() -> None: | ||||||
| config_name, action = actions[index] | ||||||
| if session := self.session_by_name(config_name): | ||||||
| session.run_code_action_async(action, progress=True, view=self.view) \ | ||||||
| .then(lambda response: self._handle_response_async(config_name, response)) | ||||||
| response = await session.run_code_action(action, progress=True, view=self.view) | ||||||
| self._handle_response_async(config_name, response) | ||||||
|
|
||||||
| sublime.set_timeout_async(run_async) | ||||||
| run_coroutine_threadsafe(run()) | ||||||
|
|
||||||
| def _handle_response_async(self, session_name: str, response: Any) -> None: | ||||||
| if isinstance(response, Error): | ||||||
|
|
@@ -463,7 +465,7 @@ def is_enabled(self, index: int, event: dict | None = None) -> bool: | |||||
| def is_visible(self, index: int, event: dict | None = None) -> bool: | ||||||
| if index == -1: | ||||||
| if self._has_session(event): | ||||||
| sublime.set_timeout_async(partial(self._request_menu_actions_async, event)) | ||||||
| call_soon_threadsafe(partial(self._request_menu_actions_async, event)) | ||||||
| return False | ||||||
| return index < len(self.actions_cache) and self._is_cache_valid(event) | ||||||
|
|
||||||
|
|
@@ -488,14 +490,14 @@ def want_event(self) -> bool: | |||||
| return True | ||||||
|
|
||||||
| def run(self, index: int, event: dict | None = None) -> None: | ||||||
| sublime.set_timeout_async(partial(self.run_async, index, event)) | ||||||
| run_coroutine_threadsafe(self._run(index, event)) | ||||||
|
|
||||||
| def run_async(self, index: int, event: dict | None) -> None: | ||||||
| async def _run(self, index: int, event: dict | None) -> None: | ||||||
| if self._is_cache_valid(event): | ||||||
| config_name, action = self.actions_cache[index] | ||||||
| if session := self.session_by_name(config_name): | ||||||
| session.run_code_action_async(action, progress=True, view=self.view) \ | ||||||
| .then(lambda response: self._handle_response_async(config_name, response)) | ||||||
| response = await session.run_code_action(action, progress=True, view=self.view) | ||||||
| self._handle_response_async(config_name, response) | ||||||
|
|
||||||
| def _handle_response_async(self, session_name: str, response: Any) -> None: | ||||||
| if isinstance(response, Error): | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.