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

[READY] Fix TypeScript unicode tests on Windows #1

Merged

Conversation

micbou
Copy link

@micbou micbou commented Apr 18, 2016

See the commit message for the details.

To be fair, it should have be done this way in the first place.


This change is Reviewable

TSServer adds a newline at the end of the response message and counts
it as one character (\n) towards the content length. But newlines are
two characters on Windows (\r\n). To take care of that, the
universal_newlines option is set when starting the TSServer
subprocess. This option automatically converts Windows newlines to \n.
However, it also opens the stdin, stdout, and stderr as text streams
instead of binary ones. This does not work properly with unicode
characters on Windows and Python 3. Therefore, we directly increment
the content length if we are on Windows instead of using the
universal_newlines option.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 84.74% when pulling 516cab5 on micbou:unicode-typescript into 24f0f21 on puremourning:unicode-investigation.

@puremourning
Copy link
Owner

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


ycmd/completers/typescript/typescript_completer.py, line 197 [r1] (raw file):
One problem we have is that we have no control over the TSServer version (we don't ship it as a submodule). Would it be possible to, say:

  • read 'Content-length' bytes
  • if on windows, and the last char is \r, read an extra byte

Does that solve it in the case of both broken and fixed tsserver?

Actually, right now, we just don't care, because that bug is still not fixed.


Comments from Reviewable

@puremourning
Copy link
Owner

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@puremourning puremourning merged commit a80cc45 into puremourning:unicode-investigation Apr 19, 2016
@puremourning
Copy link
Owner

Thanks!!

@micbou
Copy link
Author

micbou commented Apr 19, 2016

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


ycmd/completers/typescript/typescript_completer.py, line 197 [r1] (raw file):
I didn't think of that. Something like:

content = self._tsserver_handle.stdout.readline( contentlength )
if utils.OnWindows() and content.endswith( b'\r' ):
   content += self._tsserver_handle.stdout.read( 1 )

does the job. You can amend my commit if you want (and remove the TODO).


Comments from Reviewable

@micbou micbou deleted the unicode-typescript branch May 1, 2016 17:05
homu added a commit to ycm-core/ycmd that referenced this pull request May 21, 2016
[READY] Improve newline TypeScript fix on Windows

This change was suggested by @puremourning in PR puremourning#1.

With this change, we have nothing to do if issue microsoft/TypeScript#3403 is fixed and we will still support TSServer versions without the fix.

Also, according to this comment in the code:
> The response message is a JSON object which comes back on one line. Since this might change in the future, we use the 'Content-Length' header.

we should use `read` instead of `readline`.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/503)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants