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

only send format editings when necessary #420

Merged
merged 1 commit into from
Jun 9, 2022

Conversation

tw4452852
Copy link
Contributor

If the original document is same as the formatted one, there's no need to send the unchanged
document's content back which will make the client confused.

@SuperAuguste
Copy link
Member

This change is unnecessary and will also brick some clients that will wait for a format response they will never receive. The other alternatives are to send an empty or error response, but those would break clients too, thus the status quo is better. :)

@tw4452852
Copy link
Contributor Author

tw4452852 commented Jun 7, 2022

Hi @SuperAuguste

According to the spec, if there's nothing change, we could return null result in response, what do you think?

@SuperAuguste
Copy link
Member

SuperAuguste commented Jun 7, 2022

That would work, but I still do not see why this is a necessary change. Is there an example of where the status quo behavior is problematic?

@tw4452852
Copy link
Contributor Author

I found this issue with my acme editor, it will trigger formatting on saving, but every time it gets a non-empty response, it will mark the current file dirty and format the content although the formatted one and original one are the same. It's kind of annoying and I didn't encounter this problem when using another language server (e.g. gopls, rust-analyzer).

@SuperAuguste
Copy link
Member

I see! If you make format return a null instead of nothing, I'll merge this PR. Thanks!

@SuperAuguste SuperAuguste reopened this Jun 7, 2022
If the original document is same as the formatted one, there's no need to send the unchanged
document's content back which will make the client confused.

Signed-off-by: Tw <tw19881113@gmail.com>
@tw4452852
Copy link
Contributor Author

tw4452852 commented Jun 7, 2022

Hi @SuperAuguste

I just rebased the patch onto the latest master and change the behavior to respond with a null result when no changes.

@SuperAuguste
Copy link
Member

SuperAuguste commented Jun 9, 2022

LGTM, thank you so much for your contribution! Hopefully this fixes the problems you and others were facing with zls in acme. :)

@SuperAuguste SuperAuguste merged commit 769fecf into zigtools:master Jun 9, 2022
@tw4452852
Copy link
Contributor Author

Yeah, feel happy if my fix could help others.

@tw4452852 tw4452852 deleted the formatting branch June 9, 2022 03:56
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.

2 participants