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.

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

Conversation

jwortmann
Copy link
Member

This might be not very relevant in practice, because lsp_save is 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_save is meant to be a replacement for save.

@@ -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.

@rchl rchl merged commit 856fed7 into sublimelsp:main Dec 28, 2023
4 checks passed
@jwortmann jwortmann deleted the save-args branch December 29, 2023 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants