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

Add support for multiRange formatting #163190

Merged
merged 5 commits into from Mar 30, 2023

Conversation

c-claeys
Copy link
Contributor

Implement support for range formatting providers to format a list of ranges at once instead of sequentially.

This change can be validated by registering a document range formatting edit provider which leverages the new multiRange metadata property. This PR addresses #158776 and as noted in that issue, results in major performance improvements if using LSP based range formatting with a remote language server. The generic typing used ensures this change is backwards compatible. If there are no major concerns with this PR, I'll follow with one for https://github.com/microsoft/vscode-languageserver-node

@c-claeys
Copy link
Contributor Author

@microsoft-github-policy-service agree company="Google"

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't accept direct changes to the API (vscode.d.ts). Every change has to go through the proposal process and every proposal must be enforced. See https://github.com/microsoft/vscode/wiki/Extension-API-process.

Apart from the process that's required here. We should discuss the need for this API change and evaluate alternatives first

@c-claeys
Copy link
Contributor Author

Hey Johannes, that makes sense to me. Can you point me to more context on "debt week"? I tried searching further through the wiki and couldn't find anything.

As noted in the associated issue, we find this approach provides significant performance improvements when formatting multiple ranges via a remote language server. I'm open to alternative approaches, but I believe this current PR follows the API guidelines. I understand the API phone call and implementation in vscode.proposed...d.ts is still missing here.

@jrieken jrieken added feature-request Request for new features or functionality formatting Source formatter issues api-proposal labels Oct 21, 2022
@jrieken jrieken added this to the November 2022 milestone Oct 21, 2022
@jrieken
Copy link
Member

jrieken commented Nov 8, 2022

Discussed this with @dbaeumer and we are in favour of the following

  • have an option when registering a DocumentRangeFormattingEditProvider that says "canFormatMultipleRanges"
  • add a field to FormattingOption that contains all ranges

When invoking the provider always pass the array of ranges (even is length is 1) to provider that support it and always enforce that the returned text edits don't overlap. The latter is easy to enforce but something implementors need to be aware of - it might be easy to return overlapping edits when formatting multiple ranges.

@jrieken jrieken removed this from the November 2022 milestone Nov 15, 2022
@c-claeys
Copy link
Contributor Author

c-claeys commented Feb 3, 2023

add a field to FormattingOption that contains all ranges

I wasn't quite sure if you meant the way it's coded in the latest commit or if you meant as a flag in the options. Happy to change it if I misunderstood.

@c-claeys c-claeys requested a review from jrieken February 9, 2023 14:58
Copy link
Contributor Author

@c-claeys c-claeys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve so I guess the updates look fine to me, in the commit history. I marked the changes viewed, not sure if there's something else I need to do here.

Copy link
Member

@jrieken jrieken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @c-claeys for kicking this off and sorry for the delay. I have pushed a few changes, mostly move the proposal into its own file and a little cleanup.

@jrieken jrieken added this to the April 2023 milestone Mar 30, 2023
@jrieken jrieken enabled auto-merge March 30, 2023 15:09
@jrieken jrieken merged commit 9ab7b14 into microsoft:main Mar 30, 2023
7 checks passed
@c-claeys c-claeys deleted the multirange-formatting branch March 31, 2023 15:46
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-proposal feature-request Request for new features or functionality formatting Source formatter issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants