Finalize code_actions_on_format

[mj026]

This PR is a follow up of #2747 and #2751

An extra task CodeActionsFormatOnSaveTask is added to trigger the code actions defined in code_actions_on_format when format_on_save is enabled. I also added the code_actions_on_format setting to the documentation.

There was some discussion if these code actions should also be triggered when utilising the LSP: Format Selection command.

The consensus was that we should (and even take lsp_format_on_paste in account) so I investigated and tested some scenario's with this and came to the conclusion that I think we should not do this, at least for now.

The code actions that make sense to configure (and are currently allowed) in lsp_code_actions_on_save and lsp_code_actions_on_format are source.* code actions. For example, source.fixAll,source.organizeImports, source.sort.json etc.

This is also enforced in the current implementation:

LSP/plugin/code_actions.py

Lines 231 to 238 in 83bc39c

	@classmethod
	def _get_code_actions(cls, view: sublime.View) -> dict[str, bool]:
	view_code_actions = cast('dict[str, bool]', view.settings().get(cls.SETTING_NAME) or {})
	code_actions = getattr(userprefs(), cls.SETTING_NAME, {}).copy()
	code_actions.update(view_code_actions)
	return {
	key: value for key, value in code_actions.items() if key.startswith('source.')
	}

According to the specification, source.* actions are applied to the whole file:

LSP/protocol/__init__.py

Lines 470 to 484 in 83bc39c

	Source = 'source'
	"""
	Base kind for source actions: `source`
	Source code actions apply to the entire file.
	"""
	SourceOrganizeImports = 'source.organizeImports'
	"""Base kind for an organize imports source action: `source.organizeImports`"""
	SourceFixAll = 'source.fixAll'
	"""
	Base kind for auto-fix source actions: `source.fixAll`.
	Fix all actions automatically fix errors that have a clear fix that do not require user input.
	They should not suppress errors or perform unsafe fixes such as generating new types or classes.

And this is documented as such in Sublime LSP:

LSP/sublime-package.json

Line 27 in 83bc39c

"markdownDescription": "A dictionary of code action identifiers that should be triggered on format.\n\nCode action identifiers are not officially standardized so refer to specific server's documentation on what is supported but `source.fixAll` is commonly used to apply fix-on-format code actions.\n\nThis option is also supported in syntax-specific settings and/or in the `\"settings\"` section of project files. Settings from all those places will be merged and more specific (syntax and project) settings will override less specific (from LSP or Sublime settings).\n\nOnly \"source.*\" actions are supported."

When asking the LSP for a code action while sending a specific range, it will return a response with a task to format the whole file without any range data:

:: [17:18:39.508] --> ruff textDocument/codeAction (21):

{
  "textDocument": {
    "uri": "file:///my/django/project/manage.py"
  },
  "range": {
    "start": { "line": 3, "character": 0 },
    "end": { "line": 5, "character": 0 }
  },
  "context": {
    "diagnostics": [],
    "triggerKind": 2,
    "only": ["source.organizeImports.ruff"]
  }
}

:: [17:18:39.510] <<< ruff (21) (duration: 2ms):

[
  {
    "data": "file:///my/django/project/manage.py",
    "kind": "source.organizeImports.ruff",
    "title": "Ruff: Organize imports"
  }
]

The result is that other parts of your document are changed when formatting a specific section, parts which are not inside the current selection. I find this very counter intuitive and not something I would expect.

Client side filtering will result in complex situations and messy code so I did not consider it.

I also checked this specific scenario in the Zed editor (which I "stole" the idea from) and there the code actions are actually applied, outside the current selection. I think it shouldn't and therefore I did not include this behaviour.

I'm happy to see / read your thoughts about this 😅

[netlify]

✅ Deploy Preview for sublime-lsp ready!

Name	Link
🔨 Latest commit	89d191e
🔍 Latest deploy log	https://app.netlify.com/projects/sublime-lsp/deploys/698f55a9563242000899d47c
😎 Deploy Preview	https://deploy-preview-2761--sublime-lsp.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[jwortmann]

Should we add another sentence in the setting description that the code actions configured in "lsp_code_actions_on_format" are also run on save if "lsp_format_on_save" is enabled?

---

source.* actions are applied to the whole file

Right, I forgot about that. I agree that they should not be triggered then when formatting a selection.

[jwortmann]

Does this work with/without project-specific overrides?
Compare to

LSP/plugin/formatting.py

Lines 90 to 91 in 8c77738

	view_format_on_save = settings.get('lsp_format_on_save', None)
	enabled = view_format_on_save if isinstance(view_format_on_save, bool) else userprefs().lsp_format_on_save

which checks both values from the view's setting and from the global userprefs.

[mj026]

Yes, it seems like view.settings() already merges default/user/project settings. I've been testing this feature all the time with a project override without issues and this is supposed to be the expected behaviour as confirmed on the Sublime Forum.

[jwortmann]

I'm aware that ST settings from Preferences.sublime-settings are accessible in the view's settings, but userprefs() are the settings from LSP.sublime-settings. I believe we need to check those if set to true by the user.

Perhaps we should refactor that into a small function for reading LSP settings including project overrides, and then use that function here and at other places where we read such settings.

[mj026]

Ah ok, now I understand LSP.sublime-settings are not automatically taken into account. I added a new commit to check for the userprefs() setting too (and added this to the tests)

[jwortmann]

I would rewrite this method to return early if possible and you could also make use of the implementation in the base class.

Something like

if not super().is_applicable(view):
    return False
format_on_save_enabled = ...
return format_on_save_enabled

[mj026]

I changed the implementation as requested. I left out the specific userprefs() call as it doesn't seems to be explicitly needed. I check for the userprefs() setting now.

[jwortmann]

You don't really need that variable here, my code was just an example if it takes multiple lines.
Also the bool seems unnecessary because the setting is already a bool. If the type cannot be inferred correctly (Any), I would vote to either use cast or a pyright: ignore[...] comment here. (Maybe we can figure out later how to properly type view settings for the setting names from LSP.)

Also this behaves a bit different to the other place in

LSP/plugin/formatting.py

Lines 90 to 91 in 8c77738

	view_format_on_save = settings.get('lsp_format_on_save', None)
	enabled = view_format_on_save if isinstance(view_format_on_save, bool) else userprefs().lsp_format_on_save

If someone puts "lsp_format_on_save": 42 in project-specific settings, that code ignores it and uses the default / global setting, while here it would be considered true. I think I'd prefer ignoring non-bool values, but in any case I'd say it should be consistent accross different places in the code, because otherwise it can lead to difficult to find bugs or issue reports later.

---

(Also imo the comment 2 lines above is not really needed)

[mj026]

I changed the implementation now to :

    @classmethod
    @override
    def is_applicable(cls, view: sublime.View) -> bool:
        if not super().is_applicable(view):
            return False
        view_format_on_save = view.settings().get('lsp_format_on_save', None)
        return view_format_on_save if isinstance(view_format_on_save, bool) else userprefs().lsp_format_on_save

This:

• is consistent with the implementation in FormattingOnSaveTask
• will respect a boolean setting for lsp_format_on_save and ignore any other value
• returns the userprefs().lsp_format_on_save value when lsp_format_on_save is not a boolean

[jwortmann]

Yes, I'll add explanation of this behaviour to the description.

Actually I meant the LSP.sublime-settings file, because that's what most people probably read. Maybe add a sentence there too?

---

By the way. If someone puts for example

  "lsp_code_actions_on_format": {
    "source.fixAll": true,
    "source.organizeImports": true,
  },
  "lsp_code_actions_on_save": {
    "source.organizeImports": true,
  },

And also has "lsp_format_on_save" enabled, do we try to run the organizeImports code action twice when saving a file? Or could/should we skip the kinds that are already defined in "lsp_code_actions_on_save" when it gets triggered on save due to format-on-save?

[mj026]

Actually I meant the LSP.sublime-settings file, because that's what most people probably read. Maybe add a sentence there too?

Ok, I'll adjust the LSP.sublime-settings as well, will follow soon(ish)
EDIT updated the LSP.sublime-settings settings file now.

By the way. If someone puts for example

  "lsp_code_actions_on_format": {
    "source.fixAll": true,
    "source.organizeImports": true,
  },
  "lsp_code_actions_on_save": {
    "source.organizeImports": true,
  },

And also has "lsp_format_on_save" enabled, do we try to run the organizeImports code action twice when saving a file?

With the current implementation yes. The actions defined in lsp_code_actions_on_save will run before the actions in lsp_code_actions_on_format as defined in the tasks method:

def tasks(self) -> list[type[LspTask]]:
    return [
        CodeActionsOnSaveTask,
        CodeActionsFormatOnSaveTask,
        FormattingOnSaveTask,
        WillSaveWaitTask,
    ]

Or could/should we skip the kinds that are already defined in "lsp_code_actions_on_save" when it gets triggered on save due to format-on-save?

Good and relevant question (and also asked by @rchl earlier) and I decided until now to not do it as it might complicate things a bit too much.

But we could skip the code actions which are configured via lsp_code_actions_on_save indeed. I leave it up to you to decide. This seems like a good solution to prevent unneeded duplicate code actions. I'll add this functionality the following day(s).
EDIT I added this now in commit dc45572

[jwortmann]

I think this class could inherit from CodeActionsOnFormatTask, because it's a special kind of that task, which only runs on save and with the additional logic to filter out duplicated code action kinds. Then you can remove SETTING_NAME in this class.

And then we could also get rid of the @final decorator for CodeActionsOnFormatTask and for this class, which seems like a mistake to me here anyway.
@final is useful to prevent overrides, for example in

LSP/plugin/core/input_handlers.py

Lines 69 to 75 in 8c77738

	@final
	def list_items(self) -> ListItemsReturn:
	if self._initial_value is not None:
	sublime.set_timeout(self._select_and_reset)
	return [self._initial_value], 0 # pyright: ignore[reportReturnType]
	else:
	return self.get_list_items()

where list_items is normally something that you need to override in the implementation, but not in this case where it is already implemented internally and must not be changed in any subclass.

LSP/plugin/core/input_handlers.py

Lines 52 to 53 in 8c77738

	Subclasses of PreselectedListInputHandler must not implement the `list_items` method, but instead `get_list_items`,
	i.e. just prepend `get_` to the regular `list_items` method.

I don't see any reason why it would be useful on this task classes here.

[jwortmann]

Small nitpick: I would probably also rename to CodeActionsOnFormatOnSaveTask to be consistent with the other naming conventions.

Edit: I see now the other one is named FormattingOnSaveTask. Should we rename that to FormatOnSaveTask? The setting is also named "lsp_format_on_save" and not "lsp_formatting_on_save"...

[rchl]

Not having @final can make pyright/basedpyright complain if the class has class properties (or sets properties in __init__) since it then assumes that the class can be subclassed which then could lead to different types for existing properties.

I would say that it makes sense to have @final for classes that are not to be subclassed.

[jwortmann]

Not having @final can make pyright/basedpyright complain if the class has class properties (or sets properties in __init__) since it then assumes that the class can be subclassed which then can lead to different types for existing properties.

That seems weird, because we use class properties all the time, for example in

LSP/plugin/core/registry.py

Lines 103 to 115 in 8c77738

	class LspTextCommand(sublime_plugin.TextCommand):
	"""
	Inherit from this class to define your requests that should be triggered via the command palette and/or a
	keybinding.
	"""
	# When this is defined in a derived class, the command is enabled only if there exists a session with the given
	# capability attached to the active view.
	capability = ''
	# When this is defined in a derived class, the command is enabled only if there exists a session with the given
	# name attached to the active view.
	session_name = ''

where they should explicitly be overriden in subclasses.

I would say that it makes sense to have @final for classes that are not to be subclassed.

But in this case it does make sense to have a class hierarchy and allow subclasses, no? So that the subclass can reuse the SETTING_NAME from the superclass...

[rchl]

It's not that it's not allowed to have those class properties but not having explicit type can cause warning:

Type annotation for attribute capability is required because this class is not decorated with @final (basedpyright)

This is the reportUnannotatedClassAttribute setting that is currently disabled in the project. Also it's specific to basedpyright.

[rchl]

But in this case it does make sense to have a class hierarchy and allow subclasses, no? So that the subclass can reuse the SETTING_NAME from the superclass...

In that case it might make more sense to just add type annotation I suppose. But if the class is not currently subclassed by anything and the class is meant to be internal then we might just as well add @final

[jwortmann]

basedpyright's default rules are kinda ridiculous anyway because it marks almost everything as a warning.

My opinion on this is that we should use @final only if there is a particular reason that a class or a method must not be overridden, and don't just throw it on everything that currently doesn't have a subclass (yet).

Also in this case there seems already to be an explicit type annotation for that class property (not sure if that gets propagated or should get propagated to the subclass automatically):

LSP/plugin/code_actions.py

Lines 223 to 224 in 8c77738

	SETTING_NAME: str
	"""Override in your subclass to specific `lsp_code_actions_on_*` setting that should be read."""

[mj026]

It doesn't get propagated to the subclass, so I explicitly re-annotated the property in the subclasses now. See also my comment below #2761 (comment)

[rchl]

@mj026 there should be no need for you to force-push changes. This just makes it harder to review incrementally and at the end the whole PR is squashed anyway.

[rchl]

Not having @final can make pyright/basedpyright complain if the class has class properties (or sets properties in __init__) since it then assumes that the class can be subclassed which then could lead to different types for existing properties.

I would say that it makes sense to have @final for classes that are not to be subclassed.

[mj026]

@mj026 there should be no need for you to force-push changes. This just makes it harder to review incrementally and at the end the whole PR is squashed anyway

@rchl Ok, sorry for messing it up, I will not force push anymore when not needed.

Edit: I see now the other one is named FormattingOnSaveTask. Should we rename that to FormatOnSaveTask? The setting is also named "lsp_format_on_save" and not "lsp_formatting_on_save"...

So that the subclass can reuse the SETTING_NAME from the superclass...

@jwortmann I renamed FormattingOnSaveTask to FormatOnSaveTask and CodeActionsFormatOnSaveTask to CodeActionsOnFormatOnSaveTask. I also subclassed from CodeActionsOnFormatTask now so it inherits theSETTING_NAME attribute.

But in this case it does make sense to have a class hierarchy and allow subclasses, no? So that the subclass can reuse the SETTING_NAME from the superclass...
I would say that it makes sense to have @Final for classes that are not to be subclassed.

I have removed @final from the CodeActionsOnFormatTask as CodeActionsOnFormatOnSaveTask now inherits from it. I explicitly type annotated the SETTING_NAME everywere now so pyright will not complain about the reportUnannotatedClassAttribute rule.

[rchl]

I suppose this is a bit naive because keys could be subsets of other keys in which case those would also be redundant but it can be tweaked later if needed.

[mj026]

Could you give an example of keys from a subset while being redundant in this case?

[rchl]

I'm thinking source.fixAll.eslint is a subset of source.fixAll.

[mj026]

PS, I'd like to thank you @rchl and @jwortmann for accepting this feature and for all your time spend (late in the evening) on reviewing the PR's I submitted + all the suggestions and critical remarks on how to implement it / what to accept, highly appreciated!

I came across two smaller issues regarding this feature which I will submit some small future PR's to fix those. But the "main thing" is finished and I'm a happy that it is there! 🙏

[jwortmann]

Thanks for your contributions :)