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 formatMultipleRanges to DocumentRangeFormattingFeature #1253

Merged
merged 5 commits into from
Jun 26, 2023

Conversation

c-claeys
Copy link
Contributor

@c-claeys c-claeys commented Jun 7, 2023

Add multirange formatting support via new request textDocument/rangesFormatting

This does nothing besides break npm run compile ATM, but I'm looking for advice. Am I missing some hook to correctly include the proposed API? I received the proposed .d.ts via npx vscode-dts dev, but compile doesn't seem to pick it up AFAICT.

@dbaeumer
Copy link
Member

dbaeumer commented Jun 8, 2023

Couple of comments:

  • the client/tsconfig.json file is only a composite project file which shouldn't reference any files.

To get it work I would do the following:

  • create a typings folder next to src
  • move the d.ts into there
  • in the formatting.ts file reference the typings file using /// <reference path="../../typings/...." />

The reason here is that we need to ship the typings file as well since otherwise other users can't compile against the library.

@c-claeys
Copy link
Contributor Author

Sounds good. I was just looking at past discussions. Initially we were thinking of putting additional ranges in the formatting options but VSCode's API sync resulted in parallel functions exposed on that API. Any opinion over whether the formatting options should be modified to include ranges, if a new formatting method (textDocument/rangesFormatting) should be added, or something else?

@dbaeumer
Copy link
Member

I would got with a new request textDocument/rangesFormatting. I am not a bug fan of having this encoded in the options.

@c-claeys c-claeys marked this pull request as ready for review June 15, 2023 16:47
Copy link
Member

@dbaeumer dbaeumer left a comment

Choose a reason for hiding this comment

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

Since we add a capability to an existing client feature we should add a corresponding capability to the formatting client capabilities. A good name would be multipleRangesSupport: boolean or rangesSupport: boolean.

@@ -3448,10 +3448,34 @@ export interface DocumentRangeFormattingParams extends WorkDoneProgressParams {
options: FormattingOptions;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

We need a @SInCE tag here and a @proposed tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

/**
* Whether the server supports formatting multiple ranges at once.
*/
canFormatMultipleRanges?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

to make things consitent with other name and the client capability I would name it multipleRangesSupport or rangesSupport

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@dbaeumer
Copy link
Member

Otherwise (as always) great PR.

@c-claeys
Copy link
Contributor Author

Otherwise (as always) great PR.

Cheers! Thanks for the reviews

@c-claeys c-claeys requested a review from dbaeumer June 23, 2023 16:57
@dbaeumer
Copy link
Member

PR looks good. @c-claeys are you providing a spec change as well?

@dbaeumer dbaeumer enabled auto-merge (squash) June 26, 2023 08:47
@dbaeumer dbaeumer merged commit 7c161d5 into microsoft:main Jun 26, 2023
@c-claeys
Copy link
Contributor Author

Sure, happy to follow up with a spec change PR

@c-claeys c-claeys deleted the multirange branch September 12, 2023 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants