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

Normalize code actions indentation #20

Closed

Conversation

zardoy
Copy link

@zardoy zardoy commented Mar 16, 2023

for volarjs/services#30 (refactorings here output \t since format options are not passed)

I think vscode should do it automatically (since it already does the same for completions), but it uses different code for code actions

Comment on lines 29 to +34
if (action.command) {
action.command = parseServerCommand(action.command);
}
if (action.edit) {
normalizeCodeActionEdit(document, action);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The way servers like typescript-language-server handle that is they ensure that Configure request is always sent for the current file before getting code actions or doing other relevant requests. Is that not possible here?

Line endings is just one of the aspects. There are also other formatting choices like whether to use spaces or not, whether to use semicolons etc. So this doesn't seem like a very comprehensive solution.

Copy link
Author

Choose a reason for hiding this comment

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

@rchl I'm quite confused.

Line endings is just one of the aspects.

Since the package is intended for vscode only, we should never care about it, thats why it is always set to \n.
Other format options are already used, the only issue was formatOptions that contains indentation information

normalizeCodeActionEdit only changes indentation, I'm replicating vscode behavior: https://github.com/microsoft/vscode/blob/3059063b805ed0ac10a6d9539e213386bfcfb852/extensions/typescript-language-features/src/languageFeatures/fileConfigurationManager.ts#L59-L62

But you're right ideally it should be passed during request, but as I understood from vuejs/language-tools#2498 (comment) its currently not possible / no easy way to do it. Though, AFAICS from my use cases the current solution works ideally

Copy link
Contributor

Choose a reason for hiding this comment

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

I was indeed confused about line breaks vs. indentation.

But the same point still stands that indentation style is just one of the options passed through the Configure request so ideally that request should be made.

I don't know code base well enough to say whether it's feasible to do so from this point I'm just deferring to @johnsoncodehk to give his take and decide whether that's fine.

Copy link
Member

Choose a reason for hiding this comment

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

I hope to avoid use middleware handling it, which causes other language clients to have to follow if they want the same functionality. I think it would be best to raise this requirement to the LSP repo. Also replaceAll() may break \t in TemplateLiteral.

If this feature is important, these two approaches may work:

  1. Infer FormattingOptions from the upper and lower codes, I think there will be ready-made solutions.
  2. Write the FormattingOptions of the formatting request to tmpdir() or similar to share it.

Copy link
Author

Choose a reason for hiding this comment

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

2. Write the FormattingOptions of the formatting request to tmpdir() or similar to share it.

Isn't creating custom request such as sync format options much easier?

Though it is much easier to go with first way, so, let's infer code-style in place.

Copy link
Member

Choose a reason for hiding this comment

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

Because custom request requires downstream language clients to implement, I hope to reduce the necessary work for the language clients.

Copy link
Author

Choose a reason for hiding this comment

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

Though it is much easier to go with the first way, so, let's infer code-style in place.

Done here: volarjs/services@65c2a15 so I'm closing this pr

@zardoy zardoy closed this Mar 24, 2023
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