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 Formatting.format returns character float number #250

Merged
merged 1 commit into from
May 20, 2020

Conversation

wingyplus
Copy link
Contributor

@wingyplus wingyplus commented May 18, 2020

This bug happens because of use / 2 in advance_pos which is convert
into floating number. Fixes by using trunc/1. And add test to ensure
line and character always integer.

Fixes #249

@wingyplus wingyplus changed the title Ensure line and character should be integer Fix Formatting.format returns character float number May 18, 2020
Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Good catch! Just one suggestion.

@@ -81,7 +81,7 @@ defmodule ElixirLS.LanguageServer.Providers.Formatting do
else
# LSP contentChanges positions are based on UTF-16 string representation
# https://microsoft.github.io/language-server-protocol/specification#textDocuments
{line, col + byte_size(:unicode.characters_to_binary(char, :utf8, :utf16)) / 2}
{line, col + trunc(byte_size(:unicode.characters_to_binary(char, :utf8, :utf16)) / 2)}
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to use Kernel.div/2 instead?

@wingyplus
Copy link
Contributor Author

@axelson your suggestion was addressed. CI fail because of dialyzer test. I think this is not related with this changes.

@x-ji
Copy link

x-ji commented May 20, 2020

Didn't see the issue you opened and this PR before I opened mine (with Emacs)... #255 #256 Could somebody ensure that the pipeline is rerun (looked like a random timeout to me) and the fix for the bug is merged in either PR. Thanks.

This bug happens because of use `/ 2` in advance_pos which is convert
into floating number. Fixes by using `trunc/1`. And add test to ensure
line and character always integer.

Fixes elixir-lsp#249
@wingyplus
Copy link
Contributor Author

@x-ji commit was squash and ci are all green.

Copy link
Member

@axelson axelson left a comment

Choose a reason for hiding this comment

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

Thanks for making the change! ❤️

The CI tests are a little flaky sometimes.

@axelson axelson merged commit 76467cd into elixir-lsp:master May 20, 2020
@wingyplus wingyplus deleted the iss-249 branch May 21, 2020 03:07
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.

vim-lsp cannot format due to character in position is float type
3 participants