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

Explore Notebook formatting via Code Actions #212381

Open
Yoyokrazy opened this issue May 9, 2024 · 11 comments
Open

Explore Notebook formatting via Code Actions #212381

Yoyokrazy opened this issue May 9, 2024 · 11 comments
Assignees
Labels
notebook-code-actions Notebook editor code actions notebook-format Formatting support for notebooks on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Milestone

Comments

@Yoyokrazy
Copy link
Contributor

Yoyokrazy commented May 9, 2024

Overview

Opening this issue up as a discussion point for the current work around notebook formatting. As a way to accomplish this without the introduction of new API, the thought is to introduce a new Code Action Kind notebook.format.extensionID. Looping in some relevant notebook formatting people.
cc: @rebornix @karthiknadig @charliermarsh


Proposed Plan

  • Extension authors will provide a Code Action that wraps all of their formatting into a single action, with the kind notebook.format.xyz
    • We recommend that the xyz is unique to your extension, but this is not a restriction
    • We require that you have a complete DocumentFilter that contains notebookType
  • Only one formatting Code Action will be used against the notebook.
    • If more than one extension registers a provider for this type, users will be prompted to choose between them and define a setting "notebook.defaultFormatter": "extensionID" (mirrors the editor setting)
    • If an extension registers multiple notebook.format.xyz providers, only the first will be used.
    • If a notebook.format Code Action is applied on save, the standard provideDocumentFormatting flow will be skipped
  • The notebook.format Code Action can be applied in the following ways:
    • Setting "notebook.formatOnSave.enabled": true (this means that there will only be explicit triggering of this for saves, due to formatOnSave requiring the file to NOT be saved after a delay)
    • Command palette Format Notebook
    • Notebook Context Menu (🔜)

Rough Example

  • Extension (pseudo)Code:
export async function activate(context: vscode.ExtensionContext) {
	registerCodeActionsProvider(DocumentFilter, new ProviderName(), providedKinds);
}

class ProviderName {
	static readonly kind = "notebook.format.extensionID";
	
	public provideCodeActions(
		document: vscode.TextDocument,
		_range: vscode.Range | vscode.Selection,
		_context: vscode.CodeActionContext,
		_token: vscode.CancellationToken
	): vscode.CodeAction[] | undefined {
		const nbEdits: NotebookEdit[] | TextEdit[] = {
			// wrap all formatting edits here
		}
		
		const fix = new vscode.CodeAction(
			'Apply Notebook Formatting',
			ProviderName.kind,
		);
		fix.edit = new vscode.WorkspaceEdit();
		fix.edit.set(notebookDocument.uri, nbEdits);
		return [fix];
	}
}
  • User settings.json:
{
	"notebook.defaultFormatter": "extensionID", // extensionID
	"notebook.formatOnSave.enabled": true,
	
	"notebook.codeActionsOnSave": { // these will occur BEFORE the formatter
		"source.xyz": "explicit",
	}
}

Other relevant notes

  • Code Action Kind prefix rules
    • Code Action Providers that return a kind prefixed with notebook. will only ever be called against the 0th cell of the notebook. Additionally, the provided TextDocument will always correspond to the 0th cell in the notebook and contain a uri that can retrieve the notebook. It is the responsibility of the provider to extract the NotebookDocument from this (an example will be provided in a sample extension, coming 🔜).
    • If a Code Action is added to settings under notebook.codeActionsOnSave without the notebook. prefix, the codeaction will be applied to every valid cell asynchronously, in parallel.
  • Using NotebookEdit.replaceCells() is less performant than simply using TextEdit.replace() against that document. Additionally, using NotebookEdit.replaceCells() will fully replace cells, including output. When possible, use TextEdits for editing just the content of a cell.

Questions

  • Feel free to leave any and all thoughts below 👍
@Yoyokrazy Yoyokrazy added under-discussion Issue is under discussion for relevance, priority, approach notebook-format Formatting support for notebooks notebook-code-actions Notebook editor code actions labels May 9, 2024
@Yoyokrazy Yoyokrazy added this to the May 2024 milestone May 9, 2024
@Yoyokrazy Yoyokrazy self-assigned this May 9, 2024
@charliermarsh
Copy link

Also cc'ing the other folks that have worked on Notebook support in Ruff: @snowsignal @dhruvmanila

@karthiknadig
Copy link
Member

@Yoyokrazy Can you clarify where we use fully qualified extension-id vs just the extension id? Correct me if what I got is wrong

For settings.json I am assuming it will be fully qualified extension id here (like):

"notebook.defaultFormatter": "ms-python.black-formatter"

Would VS Code provide the completion similar to editor.defaultFormatter?

For the notebook.formatter.* code action, I am assuming it is just the extension id? (like):

notebook.formatter.black-formatter

@karthiknadig
Copy link
Member

@Yoyokrazy One thing to verify is what gets added as id when Language Client registers this. I am assuming it extracts the extension ID, but there is also LS server id. This feature does not pertain to LS client, but code actions are registerable via LS client.

@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented May 16, 2024

Updates after initial support has been implemented:

Note -- This information isn't necessarily specific to notebook format code actions. This information covers code actions as a whole, including our more niche notebook format situation.

  • There were some incorrect assumptions in the functionality of extension defined Code Action Kinds. Initially we were operating under the assumption that in core functionality, as the extension registers a provider, the fully qualified extension ID is bundled into the provider object. This was incorrect, only the displayName was passed through and even then only used to display a progress message to the user.

    • Changes: The fully qualified extension id (publisher.extName) is held within the provider instance, allowing core functionality to isolate actions and providers from specific extensions.
  • There was a decent bit of confusion surrounding how the suffixes for code action kinds are used, how extensions should be defining/registering them, and how the users will use this to request specific code actions via their settings.json. After a lengthy debug process, the guidelines for extensibility are as follows:

    • As an extension author, while calling registerCodeActionsProvider(), the parameters are your document filter, the provider instance, and providedCodeActionKinds within the CodeActionProviderMetadata. The strong recommendation is that you add a unique suffix to the provided kinds that you register in this metadata. This does not need to be a fully qualified extension ID, this suffix is more informal, just acting as an avenue for users to specify which of a superset of returned codeactions they want to be run on save, etc. Furthermore, while returning a Code Action inside the provideCodeActions() implementation, you should return the code action kind again suffixed with a unique identifier to your extension. (ex: ruff correctly registers and returns source.organizeImports.ruff allowing users to specify that they want that specific extension's organize import code action -- cc/@karthiknadig)
    • Users in their settings.json can choose between the broader scoped code action kind (ex: source.organizeImports), or the narrower scoped code action kind with the suffix (ex: source.organizeImports.ruff). This lets them control whether or not they want a code action provided by a specific extension. Keep in mind it is the responsibility of the extension author to register their providers with and returning code actions as having this suffix.
      • Key point here: if a user requests either a broader OR narrower scoped kind in their codeActionsOnSave setting, they will be returned ALL code actions from providers that register that base kind (regardless of suffix). This is because while vscode gathers code action providers, this is based on the being a superset or a subset of the user requested kind. Actions are provided for each provider. However, during the filtering of provided/returned actions, only code actions that are a subset of the requested kind will be applied (ex - user defines source.organizeImports.A in code actions on save but only has a provider of that registers and also returns source.organizeImports -- a more generic kind. The code action will be provided, however will be filtered out before being applied)
  • As a more detailed example:

    • Three extensions A, B, and C all register code action providers with the metadata containing the provided kind as the broader source.organizeImports. They correctly provide/return code actions that hold the more precise Kind of source.organizeImports.A/B/C
    • Case 1: User defines "editor.codeActionsOnSave": { "source.organizeImports": "always" }. Whenever they trigger an onSave event, actions from all three extensions will provided, due to the hierarchical structure of Code Action Kinds. They will also all be applied to the document due to their kinds matching the broader requested action in their settings.
    • Case 2: User defines "editor.codeActionsOnSave": { "source.organizeImports.B": "always" }. Whenever they trigger an onSave event, actions from all extensions are provided. However, only the action from extension B is applied, due to the provided action holding the narrower type source.organizeImports.B

As always, feel free to leave any and all questions!

@justschen
Copy link
Contributor

justschen commented May 16, 2024

related: #161284

the end goal would be that extension authors contribute a specific source.... action, and we'd be able to show their specific contribution in the settings.json/UI.

keep in mind https://github.com/microsoft/vscode/blob/11c70a6358044ca3615e3017c222083bda4874a7/src/vs/workbench/contrib/codeEditor/browser/saveParticipants.ts#L337C2-L342C17

private createCodeActionsOnSave(settingItems: readonly string[]): HierarchicalKind[] {
		const kinds = settingItems.map(x => new HierarchicalKind(x));

		// Remove subsets
		return kinds.filter(kind => {
			return kinds.every(otherKind => otherKind.equals(kind) || !otherKind.contains(kind));
		});
	}

if users have both source.organizeImports and source.organieImports.isort in settings.json, it just runs source.organizeImports

@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented May 16, 2024

Another example:

  • Three extensions A, B, and C all register code action providers with the metadata containing the provided kind as the narrower source.organizeImports.A/B/C. They ALSO provide/return code actions that hold the more precise Kind of source.organizeImports.A/B/C
  • User defines "editor.codeActionsOnSave": { "source.organizeImports.B": "always" }. Whenever they trigger an onSave event, only provider B is retrieved, due to nothing else matching a superset or subset. The returned code action from extension B's provider will applied to the document due to the kind matching a subset (in this case they are the same) of the requested action in their settings.

@Yoyokrazy
Copy link
Contributor Author

Also related to: #174295

This does not solve the corrupted code issue from running multiple providers against the same document, and getting edits from both. However, this information does outline the more precise way for providers to register their provided Kinds, allowing users more granular control over the execution of code actions.

On another note, referring to the following:

if users have both source.organizeImports and source.organieImports.isort in settings.json, it just runs source.organizeImports

There is currently no way for a user to disable a specific code action, if they request a broader scope in settings.json. We have seen users try the following which will not work:

{
    "editor.codeActionsOnSave": { 
        "source.organizeImports.A": "never" 
        "source.organizeImports": "always" 
    }
}

This is the user trying to opt-out of a specific action running under the "organize imports" group. This is not a functionality that is supported. In this case the user should remove their broader kind, and simply opt-in to the the providers that they specifically want.

@T-256
Copy link

T-256 commented May 23, 2024

Ruff has applyFormat command that is now format entire notebook. see astral-sh/ruff#11493.

Just dropping my 2cents here:

Non-breaking proposal:

{
    # only ruff organizes imports
    "notebook.codeActionsOnSave": { 
        "source.organizeImports.ruff": "always"
    }

    # all except ruff organizes imports
    "notebook.codeActionsOnSave": { 
        "source.organizeImports.ruff": "never",
        "source.organizeImports": "always"
    }

    # a `codeActionKind` called `sourceFormat` which will deprecate `formatOnSave` and `defaultFormatter` settings.
    "notebook.codeActionsOnSave": {
        "source.format": "always" 
    }
}

Breaking proposal:

{
    "notebook.codeActionsOnSave": {
        "source.format.ruff": "explicit",
        "source.organizeImports.ruff": "always",
        "source.organizeImports.isort": "always",
        "source.fixAll.ruff": "explicit",
    }
}

Pure code actions NOT allowed. (e.g. { "source.organizeImports": "always" })
Code actions will perform on a queue (ordered-dict) and each action will perform based on previous action result.
Possible values are only always and explicit.

@Yoyokrazy
Copy link
Contributor Author

@T-256 thanks for the input! A lot of the points you brought up have made their way through discussions on the team as we've planned this out, and I agree with a lot of the logic behind them. Unfortunately, there's been a lot of work done throughout the years and enough users (+ our product) rely on the current behavior of code action kinds that we can't adopt some of the behaviors that you brought up. Specifically the thoughts you outlined with the following code block:

    # the way code action kinds work heirarchically prevents this opt-out behavior, since they work in supersets as well as subsets
    "notebook.codeActionsOnSave": { 
        "source.organizeImports.ruff": "never",
        "source.organizeImports": "always"
    }

We're going to be trying to write enough documentation that extension authors provide the correct code action kinds to begin with. The thinking being that introducing this structure as a layer on top of the existing code action logic will prevent us from breaking any existing workflows, while giving people like ruff the rich support to develop better notebook formatting support.

@Yoyokrazy
Copy link
Contributor Author

Also wanted to check with @snowsignal @dhruvmanila to see if there is any plan or timeline for using vscode api + the new notebook format code action kind with ruff. Feel free to reach out if there's anything I can do to help out 👍

@snowsignal
Copy link

@Yoyokrazy We don't have a timeline for that yet, but we'll keep you updated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notebook-code-actions Notebook editor code actions notebook-format Formatting support for notebooks on-testplan under-discussion Issue is under discussion for relevance, priority, approach
Projects
None yet
Development

No branches or pull requests

7 participants