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 Level formatting support + api #207334

Closed
Yoyokrazy opened this issue Mar 11, 2024 · 3 comments
Closed

Explore Notebook Level formatting support + api #207334

Yoyokrazy opened this issue Mar 11, 2024 · 3 comments
Assignees
Labels
feature-request Request for new features or functionality notebook-format Formatting support for notebooks
Milestone

Comments

@Yoyokrazy
Copy link
Contributor

Tracking issue to follow progress for native notebookk formatting support within VS Code. Will need proposed api alongside it, as well as api commonds on the EH side.

@Yoyokrazy Yoyokrazy added the notebook-format Formatting support for notebooks label Mar 11, 2024
@Yoyokrazy Yoyokrazy added this to the March 2024 milestone Mar 11, 2024
@Yoyokrazy Yoyokrazy self-assigned this Mar 11, 2024
@Yoyokrazy Yoyokrazy modified the milestones: March 2024, April 2024 Mar 26, 2024
@Yoyokrazy Yoyokrazy added the feature-request Request for new features or functionality label Apr 2, 2024
@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented Apr 8, 2024

Currently we have the following two commands:

  • notebook.format

    registerAction2(class extends Action2 {
    constructor() {
    super({
    id: 'notebook.format',
    title: localize2('format.title', 'Format Notebook'),
    category: NOTEBOOK_ACTIONS_CATEGORY,
    precondition: ContextKeyExpr.and(NOTEBOOK_IS_ACTIVE_EDITOR, NOTEBOOK_EDITOR_EDITABLE),
    keybinding: {
    when: EditorContextKeys.editorTextFocus.toNegated(),
    primary: KeyMod.Shift | KeyMod.Alt | KeyCode.KeyF,
    linux: { primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KeyI },
    weight: KeybindingWeight.WorkbenchContrib
    },
    f1: true,
    menu: {
    id: MenuId.EditorContext,
    when: ContextKeyExpr.and(EditorContextKeys.inCompositeEditor, EditorContextKeys.hasDocumentFormattingProvider),
    group: '1_modification',
    order: 1.3
    }
    });
    }
    async run(accessor: ServicesAccessor): Promise<void> {
    const editorService = accessor.get(IEditorService);
    const textModelService = accessor.get(ITextModelService);
    const editorWorkerService = accessor.get(IEditorWorkerService);
    const languageFeaturesService = accessor.get(ILanguageFeaturesService);
    const bulkEditService = accessor.get(IBulkEditService);
    const editor = getNotebookEditorFromEditorPane(editorService.activeEditorPane);
    if (!editor || !editor.hasModel()) {
    return;
    }
    const notebook = editor.textModel;
    const disposable = new DisposableStore();
    try {
    const allCellEdits = await Promise.all(notebook.cells.map(async cell => {
    const ref = await textModelService.createModelReference(cell.uri);
    disposable.add(ref);
    const model = ref.object.textEditorModel;
    const formatEdits = await getDocumentFormattingEditsUntilResult(
    editorWorkerService,
    languageFeaturesService,
    model,
    model.getOptions(),
    CancellationToken.None
    );
    const edits: ResourceTextEdit[] = [];
    if (formatEdits) {
    for (const edit of formatEdits) {
    edits.push(new ResourceTextEdit(model.uri, edit, model.getVersionId()));
    }
    return edits;
    }
    return [];
    }));
    await bulkEditService.apply(/* edit */allCellEdits.flat(), { label: localize('label', "Format Notebook"), code: 'undoredo.formatNotebook', });
    } finally {
    disposable.dispose();
    }
    }
    });

  • notebook.formatCell

    registerEditorAction(class FormatCellAction extends EditorAction {
    constructor() {
    super({
    id: 'notebook.formatCell',
    label: localize('formatCell.label', "Format Cell"),
    alias: 'Format Cell',
    precondition: ContextKeyExpr.and(NOTEBOOK_IS_ACTIVE_EDITOR, NOTEBOOK_EDITOR_EDITABLE, EditorContextKeys.inCompositeEditor, EditorContextKeys.writable, EditorContextKeys.hasDocumentFormattingProvider),
    kbOpts: {
    kbExpr: ContextKeyExpr.and(EditorContextKeys.editorTextFocus),
    primary: KeyMod.Shift | KeyMod.Alt | KeyCode.KeyF,
    linux: { primary: KeyMod.CtrlCmd | KeyMod.Shift | KeyCode.KeyI },
    weight: KeybindingWeight.EditorContrib
    },
    contextMenuOpts: {
    group: '1_modification',
    order: 1.301
    }
    });
    }
    async run(accessor: ServicesAccessor, editor: ICodeEditor): Promise<void> {
    if (editor.hasModel()) {
    const instaService = accessor.get(IInstantiationService);
    await instaService.invokeFunction(formatDocumentWithSelectedProvider, editor, FormattingMode.Explicit, Progress.None, CancellationToken.None, true);
    }
    }
    });

Both of these actions take an approach that doesn't really include the context of the entire notebook at once. The former takes a naive approach and simply calls down to provideDocumentFormattingEdits() for every cell in parallel. The latter does the same, albeit just for a single cell.

The ask here is to create api and infrastructure to be able to format an entire notebook at once with the context of what is in each cell. The current limitation is that provideDocumentFormattingEdits() only takes in a single TextDocument and returns a TextEdit[]. An example use-case where this is slightly limiting is when you attempt to design a formatter that might shift all imports to a central first cell, or remove duplicated import statements. With the current approach, formatting every cell in parallel, there isn't a good solution to this as when formatting a single cell you wouldn't be aware of imports in other cells.

The proposal is to rename the above commands into something that more resembles notebook.formatAllCells +notebook.formatSingleCell, then introduce a notebook.format command that will leverage the following (wip) api:

declare module 'vscode' {

	export interface NotebookFormattingEditProvider {

		/**
		 * Provide formatting edits for a whole notebook.
		 *
		 * @param notebook The NotebookDocument in which the command was invoked.
		 * @param options Options controlling formatting.
		 * @param token A cancellation token.
		 * @returns A set of Workspace edits or a thenable that resolves to such. The lack of a result can be
		 * signaled by returning `undefined`, `null`, or an empty array.
		 */
		provideNotebookFormattingEdits(notebook: NotebookDocument, options: FormattingOptions, token: CancellationToken): ProviderResult<WorkspaceEdit[]>;
	}
}

@Yoyokrazy
Copy link
Contributor Author

Yoyokrazy commented Apr 9, 2024

Feedback from API sync (cc @rebornix):

  • Seems slightly closer to a definition problem, what is a formatter, what is a CodeAction. Bit of a grey area, and they could technically all be done with a CodeAction. This is an issue within our vscode API and how formatters/codeactions were developed. Generally:
    • Formatter => only add or remove whitespace, newlines, etc.
    • CodeAction => anything that is adding or removing characters (semicolons, copyrights, spelling, organizing imports, etc.)
  • Not really a need for new api here as it can be accomplished with a notebook.source.xyz type codeaction
    • One of the most common asks is for a formatter that can extract and shift all import statements to a central top cell.
    • While it may not be performant, polished, or finished, I've already played around with the idea here.
  • LSP limitations (cc @karthiknadig) related to LSP not knowing about notebook edits (need to revist this to understand how all the pieces fit together) which is an argument in favor of a notebook formatting provider maybe
  • What tooling limitations do Ruff/Black have that prevents them from creating codeactions to accomplish these formatting needs?

@Yoyokrazy
Copy link
Contributor Author

Closing in favor of #212381

@Yoyokrazy Yoyokrazy closed this as not planned Won't fix, can't repro, duplicate, stale May 9, 2024
@microsoft microsoft locked and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request Request for new features or functionality notebook-format Formatting support for notebooks
Projects
None yet
Development

No branches or pull requests

2 participants