-
Notifications
You must be signed in to change notification settings - Fork 32.5k
Add support for multiRange formatting #163190
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
Conversation
@microsoft-github-policy-service agree company="Google" |
There was a problem hiding this 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
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. |
Discussed this with @dbaeumer and we are in favour of the following
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. |
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. |
There was a problem hiding this 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.
1769b5f
to
89f5db5
Compare
89f5db5
to
636dd22
Compare
There was a problem hiding this 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.
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