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

The notebook.codeActionsOnSave setting should work with code actions, not commands #180839

Closed
jrieken opened this issue Apr 25, 2023 · 6 comments
Closed
Assignees
Labels
important Issue identified as high-priority
Milestone

Comments

@jrieken
Copy link
Member

jrieken commented Apr 25, 2023

Testing #180765

I noticed that there's a difference in functionality between notebook.codeActionsOnSave and editor.codeActionsOnSave. While the former works with commands, the latter works with code actions. This is a conceptual mismatch that we should reconsider, as it makes notebook code actions incompatible with editor code actions. Currently, notebooks already support "normal" code actions for their cell editors (examples can be found in GHINB), and it would be best to maintain consistency with that.

Real code actions provide structured data such as workspace edits that can be previewed, selectively applied, and configured. While there is a deprecated way for a code action provider to return just commands, it's still better than only returning command IDs, as the former can be associated with an extension and computed dynamically.

Commands, on the other hand, are more general and don't have the same benefits as code actions. There's no guarantee that a command running while saving won't change the notebook at the same time, and a command can't truly participate in save by adding pre-save edits.

Ideally, we should revisit this issue, but at a minimum, we should not use the name "codeActions" for this feature and clearly mark the setting as experimental (via its name). Additionally, we should stop recommending notebook.format as a possible on-save command, as there's a proper way to handle this (notebook.formatOnSave.enabled).

@jrieken jrieken added the important Issue identified as high-priority label Apr 25, 2023
@roblourens
Copy link
Member

The editor setting also gives intellisense for code action IDs, the notebook version should do this too

image

@mjbvz
Copy link
Contributor

mjbvz commented Apr 25, 2023

I believe the longer term goal was to migrate to code actions. Until that happens though, can we either:

  • Not ship or document this setting. This is my recommendation as that way users don't start depending the command based behavior

  • As Joh suggested, give this setting a different name that clearly marks it as experimental

@Yoyokrazy
Copy link
Contributor

Yoyokrazy commented Apr 25, 2023

I noticed that there's a difference in functionality between notebook.codeActionsOnSave and editor.codeActionsOnSave

I agree there's a discrepancy between editor.codeActionsonSave and notebook.codeActionsOnSave given the current behaviors. Right now the issue is that the api that the editor uses can't be used for notebook level CodeActions which is the current goal. The plan is to propose a new API supporting notebook level CodeActions during the next iteration which will properly utilize Workspace Edits and allow for more notebook specific CodeActions, following much of the structure laid out within the vscode.d.ts CodeAction API. This would likely include adding the intellisense that @roblourens mentioned above.

Additionally, we should stop recommending notebook.format as a possible on-save command

Yep, I'll go ahead and change this to a more generic example in the settings, and I'll go ahead and mark the setting more obviously experimental as well.

@Yoyokrazy
Copy link
Contributor

After a bit of a deeper dig, it looks like there was a misunderstanding surrounding the behavior of editor.codeActionsOnSave. The thinking initially was that editor. supported commands alongside the more structured CodeActions, which is incorrect (deprecated).

The setting will be fully removed for now, until there's more complete Notebook CodeAction support.

@rebornix
Copy link
Member

@Yoyokrazy I think this is done, right?

@rebornix rebornix removed their assignment May 31, 2023
@Yoyokrazy
Copy link
Contributor

Correct, closing since the behavior has been overhauled to work properly with CodeActions.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
important Issue identified as high-priority
Projects
None yet
Development

No branches or pull requests

5 participants