Support all arguments of native save in lsp_save command #2382
Conversation
| @@ -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] | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This is why I put the
self._kwargs = kwargsline aftertask.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.
This might be not very relevant in practice, because
lsp_saveis just an internal command.But I think this implementation to just pass through the arguments is cleaner and more flexible than hardcoding a single argument, especially since
lsp_saveis meant to be a replacement forsave.