[READY] Fixes for multi-byte errors #2108

puremourning · 2016-04-09T19:26:55Z

PR Prelude

Thank you for working on YCM! :)

Please complete these steps and check these boxes (by putting an x inside
the brackets) before filing your PR:

I have read and understood YCM's CONTRIBUTING document.
I have read and understood YCM's CODE_OF_CONDUCT document.
I have included tests for the changes in my PR. If not, I have included a
rationale for why I haven't.
I understand my PR may be closed if it becomes obvious I didn't
actually perform all of these steps.

Why this change is necessary and useful

There are a number of recent errors with unicode (most of which caused by the server, see PR ycm-core/ycmd#455. In testing I fixed a number of client-side tracebacks also.

This is by no means a comprehensive set of fixes for the client - I have simply fixed those that I came across in testing.

Summary:

fixes for errors when typing in c-sharp files due to the completion done handler
fixes for FixIts to apply correctly with multi-byte characters
fixes for unicode characters in return from the omni completer

This change is

puremourning · 2016-04-09T19:28:07Z

RFC because i still need to write some tests !

Valloric · 2016-04-10T02:46:18Z

Thanks! Looks good, but like you said, needs tests.

Review status: 0 of 4 files reviewed at latest revision, 1 unresolved discussion.

python/ycm/omni_completer.py, line 93 [r2] (raw file):
The logic is different here; we used to check is words present. So the next line may now throw a KeyError.

Comments from Reviewable

micbou · 2016-04-12T11:27:17Z

I am fairly sure there are other places where we need to convert between unicode and bytes (see PR #2090).

We'll need also to manage the case where Vim encoding is not utf-8 (by default, Vim encoding on Windows is latin1)

Reviewed 3 of 4 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

python/ycm/omni_completer.py, line 99 [r2] (raw file):
The ToUnicode is what causing the bug in issue #2096. To be honest, I don't understand why this conversion to unicode is needed. We want to return bytes to Vim, not unicode because Vim is working with column byte offsets.

python/ycm/youcompleteme.py, line 380 [r2] (raw file):
Wrong indentation here.

Comments from Reviewable

Valloric · 2016-04-12T16:46:21Z

Review status: all files reviewed at latest revision, 3 unresolved discussions.

python/ycm/omni_completer.py, line 99 [r2] (raw file):
The Vim python interface accepts unicode objects in addition to bytes. The benefit of passing unicode objects is that they are correctly converted into the encoding the Vim buffer is using (I think).

Comments from Reviewable

puremourning · 2016-04-13T22:10:24Z

There almost certainly are other places. Like I said in the PR description this is not a thorough review and test of YCM, just what I came across while testing ycmd. Indeed I really only posted it in the hope that folk seeing common issues could take advantage and/or add more testing :)

The vim encoding thing is probably going to be painful. I risked :help multibyte and my eyes glazed over somewhat. Interestingly it does state that overlong sequences are not displayed correctly, so ycmd not supporting them either might be less of a deal :)

Review status: all files reviewed at latest revision, 3 unresolved discussions.

python/ycm/omni_completer.py, line 93 [r2] (raw file):
Quite right. Not only that but the check is totally bogus as is. Sorry about that, I've been concentrating on the ycmd side of things.

python/ycm/omni_completer.py, line 99 [r2] (raw file):
The issue is not the ToUnicode (I don't think). That call is required IIRC to prevent completion suggestions being b'blah', but the completely bogus check for isinstance( items, dict ). The thing returned from Vim is probably not actually a dict, so changin it to if hasattr( items, '__getitem__' ) and 'words' in items seems to solve both problems with that change.

I need to write a bunch of tests for this area, because it is actually quite fiddly.

python/ycm/youcompleteme.py, line 380 [r2] (raw file):
Good spot, thanks.

Comments from Reviewable

Valloric · 2016-04-13T22:45:18Z

I'm far less worried about correct unicode support for people using Vim with non-unicode encodings. They already probably only use ASCII. If they want a unicode-aware Vim setup, they should use some unicode encoding.

I do not expect this PR to fix all the unicode issues, just plenty of them. "Perfect is the enemy of good" and all that. Let's improve matters considerably with this PR and take it from there.

Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

Valloric · 2016-04-13T22:47:29Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

python/ycm/omni_completer.py, line 99 [r2] (raw file):
I hear "more tests" and my mind leaps to "YES". :)

Comments from Reviewable

puremourning · 2016-04-24T15:15:25Z

I think this is ready now, it just needs the ycmd PR merged first

vheon · 2016-04-24T16:47:25Z

Reviewed 1 of 4 files at r1, 9 of 10 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

.gitmodules, line 6 [r4] (raw file):
remember to restore this 😝

Comments from Reviewable

puremourning · 2016-04-24T16:48:43Z

Review status: all files reviewed at latest revision, 2 unresolved discussions.

.gitmodules, line 6 [r4] (raw file):
yeah. i will just drop the commit when the ycmd change is merged

python/ycm/omni_completer.py, line 99 [r2] (raw file):
I've totally gone back on this. The correct thing to do is indeed to remove the ToUnicode call and to reinstate the check on isinstance( items ) dict and 'words' in items. Go tests!!

Comments from Reviewable

Valloric · 2016-04-24T19:05:46Z

Is this updated for the ycmd merge of the unicode PR? Other than that, this is .

Reviewed 1 of 4 files at r1, 9 of 10 files at r3, 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

Comments from Reviewable

puremourning · 2016-04-24T22:36:26Z

Yeah I have now updated this PR to the latest upstream ycmd after the other PR is merged.

micbou · 2016-04-27T22:55:23Z

I made some comments but this looks fine.

Reviewed 9 of 10 files at r3, 3 of 3 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 11 unresolved discussions.

run_tests.py, line 78 [r5] (raw file):
Pedantic comment but since there are no blank lines around these lines in the ycmd run_tests.py script, we should do the same here.

python/ycm/omni_completer.py, line 108 [r5] (raw file):
Should we not use the VimExpressionToPythonType function from vimsupport here?

python/ycm/test_utils.py, line 60 [r4] (raw file):
I know that server_log_level and extra_conf_vim_data are enough to create a YCM object. Other options are useful for the diagnostic tests. Not a huge deal though.

python/ycm/test_utils.py, line 145 [r4] (raw file):
An instead of A.

python/ycm/test_utils.py, line 153 [r4] (raw file):
Thanks for documenting this.

python/ycm/tests/omni_completer_test.py, line 508 [r4] (raw file):
There are two spaces after the comma.

python/ycm/tests/omni_completer_test.py, line 688 [r4] (raw file):
Same comment as for the OmniCompleter_GetCompletions_Cache_List_Filter_Unicode test.

python/ycm/tests/omni_completer_test.py, line 747 [r4] (raw file):
the filtered results are?

python/ycm/tests/omni_completer_test.py, line 607 [r5] (raw file):
Should we use the ExpectedFailure decorator for this test?

python/ycm/tests/postcomplete_tests.py, line 31 [r4] (raw file):
I would move this import with the other ycm imports below.

Comments from Reviewable

homu · 2016-04-28T03:01:04Z

☔ The latest upstream changes (presumably #2140) made this pull request unmergeable. Please resolve the merge conflicts.

- Correct FixIts when there are unicode characters - Fix errors in CompleteDone handler - Fix tracebacks when omnifunc returns unicode chars

puremourning · 2016-05-08T14:46:54Z

Review status: 8 of 10 files reviewed at latest revision, 11 unresolved discussions.

run_tests.py, line 78 [r5] (raw file):

Previously, micbou wrote…

Pedantic comment but since there are no blank lines around these lines in the ycmd run_tests.py script, we should do the same here.

Not sure i totally agree, really. Making it ugly in both places seems weaker to me :)

Anyways, I don't care nearly enough to argue, so done ^_^

python/ycm/omni_completer.py, line 108 [r5] (raw file):

Previously, micbou wrote…

Should we not use the VimExpressionToPythonType function from vimsupport here?

Maybe, but it's certainly not necessary, and might lead to a subtle bug if &onmifunc happened to return "1" (which may or may not be valid). &onmifunc has to be a string, so I'm cool with just ToUnicode on it.

python/ycm/test_utils.py, line 60 [r4] (raw file):

Previously, micbou wrote…

I know that server_log_level and extra_conf_vim_data are enough to create a YCM object. Other options are useful for the diagnostic tests. Not a huge deal though.

maybe, but i don't feel there's much value in having many of these. In the real world of YCM in Vim, all of these things are set by youcompleteme.vim

python/ycm/test_utils.py, line 145 [r4] (raw file):

Previously, micbou wrote…

An instead of A.

Done.

python/ycm/test_utils.py, line 153 [r4] (raw file):

Previously, micbou wrote…

Thanks for documenting this.

I just found that I had to look for an example 2x, so was like, now is the time to compress.

python/ycm/tests/omni_completer_test.py, line 508 [r4] (raw file):

Previously, micbou wrote…

There are two spaces after the comma.

/FilterningDone.

python/ycm/tests/omni_completer_test.py, line 688 [r4] (raw file):

Previously, micbou wrote…

Same comment as for the OmniCompleter_GetCompletions_Cache_List_Filter_Unicode test.

Done.

python/ycm/tests/omni_completer_test.py, line 747 [r4] (raw file):

Previously, micbou wrote…

the filtered results are?

Done.

python/ycm/tests/omni_completer_test.py, line 607 [r5] (raw file):

Previously, micbou wrote…

Should we use the ExpectedFailure decorator for this test?

Done.

python/ycm/tests/postcomplete_tests.py, line 31 [r4] (raw file):

Previously, micbou wrote…

I would move this import with the other ycm imports below.

Done.

.gitmodules, line 6 [r4] (raw file):

Previously, puremourning (Ben Jackson) wrote…

yeah. i will just drop the commit when the ycmd change is merged

Done.

Comments from Reviewable

micbou · 2016-05-08T15:01:21Z

Previously, homu (Homu) wrote…

☔ The latest upstream changes (presumably #2140) made this pull request unmergeable. Please resolve the merge conflicts.

Reviewed 6 of 7 files at r6.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

python/ycm/omni_completer.py, line 108 [r5] (raw file):

Previously, puremourning (Ben Jackson) wrote…

Maybe, but it's certainly not necessary, and might lead to a subtle bug if &onmifunc happened to return "1" (which may or may not be valid). &onmifunc has to be a string, so I'm cool with just ToUnicode on it.

Let's keep it that way then.

Comments from Reviewable

Valloric · 2016-05-08T16:58:17Z

Good to go? Seems like it.

Thanks for the PR!

@homu r=valloric

Previously, micbou wrote…

:lgtm:

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

homu · 2016-05-08T16:58:18Z

📌 Commit e734261 has been approved by valloric

homu · 2016-05-08T16:58:29Z

⚡ Test exempted - status

[READY] Fixes for multi-byte errors # PR Prelude Thank you for working on YCM! :) **Please complete these steps and check these boxes (by putting an `x` inside the brackets) _before_ filing your PR:** - [X] I have read and understood YCM's [CONTRIBUTING][cont] document. - [X] I have read and understood YCM's [CODE_OF_CONDUCT][code] document. - [X] I have included tests for the changes in my PR. If not, I have included a rationale for why I haven't. - [X] **I understand my PR may be closed if it becomes obvious I didn't actually perform all of these steps.** # Why this change is necessary and useful There are a number of recent errors with unicode (most of which caused by the server, see PR ycm-core/ycmd#455. In testing I fixed a number of client-side tracebacks also. This is by no means a comprehensive set of fixes for the client - I have simply fixed those that I came across in testing. Summary: - fixes for errors when typing in c-sharp files due to the completion done handler - fixes for FixIts to apply correctly with multi-byte characters - fixes for unicode characters in return from the omni completer [cont]: https://github.com/Valloric/YouCompleteMe/blob/master/CONTRIBUTING.md [code]: https://github.com/Valloric/YouCompleteMe/blob/master/CODE_OF_CONDUCT.md  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2108)

micbou mentioned this pull request Apr 10, 2016

[WIP] Send column as codepoint index to ycmd #2073

Closed

puremourning force-pushed the unicode branch 3 times, most recently from 667f3d4 to 8200289 Compare April 24, 2016 14:51

puremourning changed the title ~~[RFC] Fixes for multi-byte errors~~ [READY - pending ycmd update] Fixes for multi-byte errors Apr 24, 2016

puremourning force-pushed the unicode branch from 1b1cde2 to 5456d90 Compare April 24, 2016 15:19

puremourning force-pushed the unicode branch from 5456d90 to 8fe86db Compare April 24, 2016 22:34

puremourning changed the title ~~[READY - pending ycmd update] Fixes for multi-byte errors~~ [READY] Fixes for multi-byte errors Apr 24, 2016

vheon mentioned this pull request Apr 27, 2016

Support for Unicode characters for filename completer #278

Closed

puremourning added 2 commits May 8, 2016 15:32

Fix a number of multi-byte errors and tracebacks

4d7b386

- Correct FixIts when there are unicode characters - Fix errors in CompleteDone handler - Fix tracebacks when omnifunc returns unicode chars

Add --no-flake8 to YCM run_tests.py

81db1c0

puremourning force-pushed the unicode branch 2 times, most recently from a10a049 to 3e3e9bf Compare May 8, 2016 14:46

Add tests for omni completer GetCompletions

e734261

puremourning force-pushed the unicode branch from 3e3e9bf to e734261 Compare May 8, 2016 14:48

homu merged commit e734261 into ycm-core:master May 8, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Fixes for multi-byte errors #2108

[READY] Fixes for multi-byte errors #2108

puremourning commented Apr 9, 2016

puremourning commented Apr 9, 2016

Valloric commented Apr 10, 2016

micbou commented Apr 12, 2016

Valloric commented Apr 12, 2016

puremourning commented Apr 13, 2016

Valloric commented Apr 13, 2016

Valloric commented Apr 13, 2016

puremourning commented Apr 24, 2016

vheon commented Apr 24, 2016

puremourning commented Apr 24, 2016

Valloric commented Apr 24, 2016

puremourning commented Apr 24, 2016

micbou commented Apr 27, 2016

homu commented Apr 28, 2016

puremourning commented May 8, 2016

micbou commented May 8, 2016

Valloric commented May 8, 2016

homu commented May 8, 2016

homu commented May 8, 2016

[READY] Fixes for multi-byte errors #2108

[READY] Fixes for multi-byte errors #2108

Conversation

puremourning commented Apr 9, 2016

PR Prelude

Why this change is necessary and useful

puremourning commented Apr 9, 2016

Valloric commented Apr 10, 2016

micbou commented Apr 12, 2016

Valloric commented Apr 12, 2016

puremourning commented Apr 13, 2016

Valloric commented Apr 13, 2016

Valloric commented Apr 13, 2016

puremourning commented Apr 24, 2016

vheon commented Apr 24, 2016

puremourning commented Apr 24, 2016

Valloric commented Apr 24, 2016

puremourning commented Apr 24, 2016

micbou commented Apr 27, 2016

homu commented Apr 28, 2016

puremourning commented May 8, 2016

micbou commented May 8, 2016

Valloric commented May 8, 2016

homu commented May 8, 2016

homu commented May 8, 2016