Skip to content
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.

Sign up for GitHub

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

Support all arguments of native save in lsp_save command #2382

Merged
merged 1 commit into from
Dec 28, 2023
Merged

Support all arguments of native save in lsp_save command #2382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Default.sublime-keymap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@
{
"keys": ["primary+s"],
"command": "lsp_save",
"args": {"async": true},
"context": [{"key": "lsp.session_with_capability", "operand": "textDocumentSync.willSave | textDocumentSync.willSaveWaitUntil | codeActionProvider.codeActionKinds | documentFormattingProvider | documentRangeFormattingProvider"}]
},
]
10 changes: 6 additions & 4 deletions plugin/save_command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
from .core.registry import LspTextCommand
from .core.settings import userprefs
from .core.typing import Callable, List, Type
from .core.typing import Any, Callable, Dict, List, Type
from abc import ABCMeta, abstractmethod
import sublime
import sublime_plugin
Expand Down Expand Up @@ -75,12 +75,14 @@ def register_task(cls, task: Type[SaveTask]) -> None:
def __init__(self, view: sublime.View) -> None:
super().__init__(view)
self._pending_tasks = [] # type: List[SaveTask]
self._kwargs = {} # type: Dict[str, Any]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of storing arguments like that in the instance since the command is reused between invocations and especially with async commands like this one that can technically result in incorrect behavior.

In practice I don't anticipate this causing serious issues but at the same time it should be easy to do things properly and pass those arguments around instead of storing on the instance.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking closer, I suppose if there are pending tasks then invoking the command will cancel tasks and not trigger save so I guess this is fine...

There might also be an issue in the existing code related to processing pending tasks when running this command twice in a row...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

invoking the command will cancel tasks

This is why I put the self._kwargs = kwargs line after task.cancel() in the code. So I guess it should be safe even when some tasks still execute in the background from a previous invokation. So I thought that passing around arguments via functools.partial would be unnecessary here.

There might also be an issue in the existing code related to processing pending tasks when running this command twice in a row...

Could you elaborate how this could become a problem?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I put the self._kwargs = kwargs line after task.cancel() in the code.

Yeah, should be fine in practice.

Could you elaborate how this could become a problem?

>>> view.run_command('lsp_save');view.run_command('lsp_save')
Traceback (most recent call last):
  File "/Users/rafal/Library/Application Support/Sublime Text/Packages/LSP/plugin/save_command.py", line 102, in _run_next_task_async
    current_task = self._pending_tasks[0]
IndexError: list index out of range

I guess not likely to get triggered with manual key press but still some issue.


def run(self, edit: sublime.Edit) -> None:
def run(self, edit: sublime.Edit, **kwargs: Dict[str, Any]) -> None:
if self._pending_tasks:
for task in self._pending_tasks:
task.cancel()
self._pending_tasks = []
self._kwargs = kwargs
sublime.set_timeout_async(self._trigger_on_pre_save_async)
for Task in self._tasks:
if Task.is_applicable(self.view):
Expand Down Expand Up @@ -113,7 +115,7 @@ def _on_task_completed_async(self) -> None:

def _trigger_native_save(self) -> None:
# Triggered from set_timeout to preserve original semantics of on_pre_save handling
sublime.set_timeout(lambda: self.view.run_command('save', {"async": True}))
sublime.set_timeout(lambda: self.view.run_command('save', self._kwargs))


class LspSaveAllCommand(sublime_plugin.WindowCommand):
Expand All @@ -128,4 +130,4 @@ def run(self, only_files: bool = False) -> None:
if only_files and view.file_name() is None:
continue
done.add(buffer_id)
view.run_command("lsp_save", None)
view.run_command("lsp_save", {'async': True})
12 changes: 6 additions & 6 deletions tests/test_code_actions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ def test_applies_matching_kind(self) -> Generator:
code_action_kind
)
self.set_response('textDocument/codeAction', [code_action])
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
yield from self.await_message('textDocument/codeAction')
yield from self.await_message('textDocument/didSave')
self.assertEquals(entire_content(self.view), 'const x = 1;')
Expand All @@ -134,7 +134,7 @@ def test_requests_with_diagnostics(self) -> Generator:
code_action_kind
)
self.set_response('textDocument/codeAction', [code_action])
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
code_action_request = yield from self.await_message('textDocument/codeAction')
self.assertEquals(len(code_action_request['context']['diagnostics']), 1)
self.assertEquals(code_action_request['context']['diagnostics'][0]['message'], 'Missing semicolon')
Expand Down Expand Up @@ -176,7 +176,7 @@ def test_applies_only_one_pass(self) -> Generator:
]
),
])
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
# Wait for the view to be saved
yield lambda: not self.view.is_dirty()
self.assertEquals(entire_content(self.view), 'const x = 1;')
Expand All @@ -191,7 +191,7 @@ def test_applies_immediately_after_text_change(self) -> Generator:
code_action_kind
)
self.set_response('textDocument/codeAction', [code_action])
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
yield from self.await_message('textDocument/codeAction')
yield from self.await_message('textDocument/didSave')
self.assertEquals(entire_content(self.view), 'const x = 1;')
Expand All @@ -200,7 +200,7 @@ def test_applies_immediately_after_text_change(self) -> Generator:
def test_no_fix_on_non_matching_kind(self) -> Generator:
yield from self._setup_document_with_missing_semicolon()
initial_content = 'const x = 1'
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
yield from self.await_message('textDocument/didSave')
self.assertEquals(entire_content(self.view), initial_content)
self.assertEquals(self.view.is_dirty(), False)
Expand All @@ -215,7 +215,7 @@ def test_does_not_apply_unsupported_kind(self) -> Generator:
code_action_kind
)
self.set_response('textDocument/codeAction', [code_action])
self.view.run_command('lsp_save')
self.view.run_command('lsp_save', {'async': True})
yield from self.await_message('textDocument/didSave')
self.assertEquals(entire_content(self.view), 'const x = 1')

Expand Down
6 changes: 3 additions & 3 deletions tests/test_single_document.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def test_sends_save_with_purge(self) -> 'Generator':
assert self.view
self.view.settings().set("lsp_format_on_save", False)
self.insert_characters("A")
self.view.run_command("lsp_save")
self.view.run_command("lsp_save", {'async': True})
yield from self.await_message("textDocument/didChange")
yield from self.await_message("textDocument/didSave")
yield from self.await_clear_view_and_save()
Expand All @@ -123,7 +123,7 @@ def test_formats_on_save(self) -> 'Generator':
'end': {'line': 0, 'character': 1}
}
}])
self.view.run_command("lsp_save")
self.view.run_command("lsp_save", {'async': True})
yield from self.await_message("textDocument/formatting")
yield from self.await_message("textDocument/didChange")
yield from self.await_message("textDocument/didSave")
Expand Down Expand Up @@ -384,7 +384,7 @@ def test_will_save_wait_until(self) -> 'Generator':
}
}])
self.view.settings().set("lsp_format_on_save", False)
self.view.run_command("lsp_save")
self.view.run_command("lsp_save", {'async': True})
yield from self.await_message("textDocument/willSaveWaitUntil")
yield from self.await_message("textDocument/didChange")
yield from self.await_message("textDocument/didSave")
Expand Down