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

Fix selective formatting (issue #1127) #1128

Merged
merged 2 commits into from
Oct 3, 2024

Conversation

travkin79
Copy link
Contributor

This PR fixes selective formatting.

@travkin79
Copy link
Contributor Author

I think, my fix is ready, but I'd like to add an automated test for that case, too.

@rubenporras
Copy link
Contributor

Thanks for the fix. It looks good to me but I will wait for the test case before merging.

Also adapt mock server to support range formatting and
add tests.
@travkin79
Copy link
Contributor Author

I think, now, the PR is ready to be merged. But please take a look at the following line. Is that the right thing to be returned in this case?

https://github.com/travkin79/lsp4e/blob/15350a9bff093771bc306cf06ffea2f25d9106c7/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/format/LSPFormatter.java#L66

@travkin79 travkin79 marked this pull request as ready for review October 2, 2024 15:14
@travkin79
Copy link
Contributor Author

Hi @rubenporras,
Since this bugfix is important and urgent for my team, when do you plan to make the next LSP4E release?

@rubenporras
Copy link
Contributor

I think, now, the PR is ready to be merged. But please take a look at the following line. Is that the right thing to be returned in this case?

https://github.com/travkin79/lsp4e/blob/15350a9bff093771bc306cf06ffea2f25d9106c7/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/format/LSPFormatter.java#L66

I am also not sure and you implementation seems sensible.

In any case, I think that if the client pays attention to the capabilities of the client, we should never reach this code.

@rubenporras rubenporras merged commit 0324785 into eclipse:main Oct 3, 2024
6 checks passed
@rubenporras
Copy link
Contributor

Hi @rubenporras, Since this bugfix is important and urgent for my team, when do you plan to make the next LSP4E release?

In that case, I will prepare a new release today or tomorrow.

@rubenporras
Copy link
Contributor

Hi @travkin79 ,

you have a new release ;)

Cheers.

@travkin79
Copy link
Contributor Author

travkin79 commented Oct 4, 2024

you have a new release ;)

Hi @rubenporras,
That's awesome! Thank you very much. :-)

One more thing: I think, we can close the issue #1127 now.

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