[READY] Remove unloaded_buffer parameter #542

micbou · 2016-07-12T15:36:43Z

Closes #541.

This change is

codecov-io · 2016-07-12T16:47:15Z

Current coverage is 93.62% (diff: 100%)

Merging #542 into master will increase coverage by 0.10%

@@             master       #542   diff @@
==========================================
  Files            41         41          
  Lines          3856       3856          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           3606       3610     +4   
+ Misses          250        246     -4   
  Partials          0          0

Powered by Codecov. Last update 525a971...4db361f

puremourning · 2016-07-12T20:24:46Z

That was my thought when i saw the ycmd issue :)

Don't suppose there's any way we can make a simple test for this? I guess the only real way would be to mock out _SendCommand and check that it gets the right file name. Not a big deal for this PR if not.

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

Valloric · 2016-07-12T23:38:09Z

although like @puremourning, I'd like to see some sort of test. Not necessarily in this PR.

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

Comments from Reviewable

micbou · 2016-07-14T14:37:37Z

I'd also like to add a test for this. I think the best way would be to find an example that fails without this change where we open two files in TSServer, close one of them, and ask for completions in the other. I tried to find such example but was not successful. At this point, I even wonder if closing a file in TSServer is doing anything.

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

Comments from Reviewable

micbou · 2016-09-04T15:16:36Z

I gave some thoughts to the unloaded_buffer parameter and I think we should drop it. From my understanding, it exists because the current buffer is not necessarily the one being unloaded. One could argue that in this case the client should send a request about the buffer being unloaded and not the current buffer. But that's not the main reason to drop the parameter. The main reason is that if the filetype of the current buffer is different from the unloaded buffer one then the wrong completer will be called or no completer at all if the filetype is not supported. For instance, if we are editing a TypeScript file while unloading a C++ one, we will tell TSServer to close the C++ file, which is completely wrong.

So, what do you think? Should we drop the parameter in this PR or merge it in current state and open a new PR for that?

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

Comments from Reviewable

puremourning · 2016-09-04T16:52:55Z

That's certainly a good point. I wonder what the other client maintainers think...

Does the Vim client even send this parameter as anything other than the current file?

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

Comments from Reviewable

ptrv · 2016-09-05T08:35:28Z

In Emacs we send the current file in the BufferUnload request

micbou · 2016-09-05T13:58:23Z

Does the Vim client even send this parameter as anything other than the current file?

YCM send this parameter as the unloaded buffer path.

In Emacs we send the current file in the BufferUnload request

Would it be an issue to send the unloaded/closed buffer instead in the Emacs client?

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

Comments from Reviewable

ptrv · 2016-09-05T14:09:26Z

Would it be an issue to send the unloaded/closed buffer instead in the Emacs client?

That is the case. The current file is the one which is closed. So basically in Emacs the file for unloaded_buffer and filepath in the request data is the same. Sorry if I wasn't clear enough.

micbou · 2016-09-05T14:57:31Z

Thanks. I've looked at the Atom client and it seems to do the same as the Emacs client. The Sublime client is not implementing the UnloadBuffer event notification. @Qusic and @LuckyGeck may confirm this.

In conclusion, only the Vim client is affected by the issue I mentioned above (shame on us). So, we should first fix the bug in YCM by sending the request as the unloaded buffer instead of the current one for the BufferUnload event notification and then drop the unloaded_buffer parameter in ycmd.

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

Comments from Reviewable

Qusic · 2016-09-06T02:47:39Z

I've looked at the Atom client and it seems to do the same as the Emacs client.

Yes. So Atom client won't be affected.

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

Comments from Reviewable

[READY] Use deleted buffer instead of current buffer when sending BufferUnload event notification When deleting a buffer, we use the current buffer to send the `BufferUnload` event notification request to ycmd instead of the buffer being deleted. This is an issue when the current buffer is not of the same filetype as the deleted one. In this case, three different scenarios may occur: - the current buffer filetype is not allowed: no request is sent to the ycmd server. The `OnBufferUnload` method from the completer corresponding to the filetype of the deleted buffer is not called. If the filetype is a C-family language, the translation unit is not destroyed. If it is TypeScript, we don't tell TSServer to close the file (not really an issue unless the file is modified elsewhere); - the current buffer filetype has no semantic support in ycmd: the request is sent to the ycmd server but no semantic completer is used. Same result; - the current buffer filetype has semantic support in ycmd: the `OnBufferUnload` method from the wrong completer is called in ycmd. LibClang and TSServer are able to cope with that and ignore the file. Same result in definitive. The solution is obvious: build the request for the deleted buffer in this case. However, this involves more changes than expected because the code was written with the assumption that requests are always for the current buffer. The `include_buffer_data` parameter from `BuildRequestData` was removed as it is always used with its default value. This fix may alleviate issue #2232 in the sense that it now gives a reliable way to limit memory usage by deleting buffers in the case of multiple TUs. Next step is to update PR ycm-core/ycmd#542 by removing the `unloaded_buffer` parameter from ycmd API then remove it from the `BufferUnload` request in YCM.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2367)

micbou · 2016-10-24T22:16:40Z

I updated the PR by completely removing the unloaded_buffer parameter. I also added a test covering the BufferUnload event of the TypeScript completer. This test illustrates the limits of our completions caching system and a deficiency in how the TypeScript completer deal with dirty buffers (see the FIXME annotations).

PR is now ready.

Reviewed 1 of 5 files at r4, 3 of 3 files at r5, 3 of 3 files at r7, 1 of 1 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

Valloric · 2016-10-25T01:20:16Z

as before.

Review status: 3 of 4 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

vheon · 2016-10-25T21:09:23Z

I am a little puzzled by the title of the PR and the code of the PR so, we were already closing the right buffer in the Typescript completer?

@micbou the code so if is only a matter of an "old" title feel free to fire homu yourself.

Review status: 3 of 4 files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

vheon · 2016-10-25T21:09:45Z

Reviewed 1 of 5 files at r4, 2 of 3 files at r7, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

micbou · 2016-10-25T21:20:38Z

I forgot to update the title of the PR. Sorry for the confusion.

@homu r=vheon

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

Comments from Reviewable

homu · 2016-10-25T21:20:40Z

📌 Commit 210360f has been approved by vheon

[READY] Remove unloaded_buffer parameter Closes #541.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/542)

homu · 2016-10-25T21:20:56Z

⌛ Testing commit 210360f with merge b004c17...

homu · 2016-10-25T23:18:54Z

💔 Test failed - status

Remove unloaded_buffer parameter from BufferUnload event notification request.

vheon · 2016-10-26T16:06:42Z

@homu retry

vheon · 2016-10-26T16:09:45Z

@homu r+

homu · 2016-10-26T16:09:47Z

📌 Commit ba85bed has been approved by vheon

[READY] Remove unloaded_buffer parameter Closes #541.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/542)

homu · 2016-10-26T16:10:01Z

⌛ Testing commit ba85bed with merge 3cb6fb0...

micbou · 2016-10-26T16:42:20Z

There is an issue with the TypeScript tests on Linux Travis. I'm on it.

homu · 2016-10-26T17:15:46Z

💔 Test failed - status

micbou · 2016-10-26T22:11:06Z

@homu retry

[READY] Remove unloaded_buffer parameter Closes #541.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/542)

homu · 2016-10-26T22:11:19Z

⌛ Testing commit ba85bed with merge c3daaac...

homu · 2016-10-26T23:39:02Z

☀️ Test successful - status

[READY] Remove unloaded_buffer parameter See PR ycm-core/ycmd#542.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2407)

micbou changed the title ~~[READY] Close unloaded buffer instead of current one for BufferUnload event in TypeScript completer~~ [WIP] Close unloaded buffer instead of current one for BufferUnload event in TypeScript completer Jul 14, 2016

micbou force-pushed the typescript-unloaded-buffer branch 2 times, most recently from f5f2c67 to e7af3c5 Compare September 3, 2016 23:22

micbou mentioned this pull request Oct 4, 2016

[READY] Use deleted buffer instead of current buffer when sending BufferUnload event notification ycm-core/YouCompleteMe#2367

Merged

micbou force-pushed the typescript-unloaded-buffer branch 2 times, most recently from 57a0110 to e331832 Compare October 8, 2016 19:09

micbou force-pushed the typescript-unloaded-buffer branch from e331832 to fc9eca3 Compare October 12, 2016 21:41

micbou force-pushed the typescript-unloaded-buffer branch 3 times, most recently from a4d394f to 8f9219c Compare October 24, 2016 21:34

micbou changed the title ~~[WIP] Close unloaded buffer instead of current one for BufferUnload event in TypeScript completer~~ [READY] Close unloaded buffer instead of current one for BufferUnload event in TypeScript completer Oct 24, 2016

micbou force-pushed the typescript-unloaded-buffer branch from 8f9219c to 210360f Compare October 24, 2016 22:54

micbou changed the title ~~[READY] Close unloaded buffer instead of current one for BufferUnload event in TypeScript completer~~ [READY] Remove unloaded_buffer parameter Oct 25, 2016

micbou force-pushed the typescript-unloaded-buffer branch from 210360f to 4db361f Compare October 25, 2016 23:41

micbou added 2 commits October 26, 2016 02:52

Remove unloaded_buffer parameter

9df02c0

Remove unloaded_buffer parameter from BufferUnload event notification request.

Add test for BufferUnload event in TypeScript completer

ba85bed

micbou force-pushed the typescript-unloaded-buffer branch from 4db361f to ba85bed Compare October 26, 2016 00:53

micbou changed the title ~~[READY] Remove unloaded_buffer parameter~~ [WIP] Remove unloaded_buffer parameter Oct 26, 2016

micbou changed the title ~~[WIP] Remove unloaded_buffer parameter~~ [READY] Remove unloaded_buffer parameter Oct 26, 2016

ptrv mentioned this pull request Oct 26, 2016

Adapt to upstream ycmd changes abingham/emacs-ycmd#373

Merged

homu merged commit ba85bed into ycm-core:master Oct 26, 2016

micbou deleted the typescript-unloaded-buffer branch October 26, 2016 23:41

micbou mentioned this pull request Oct 26, 2016

[READY] Remove unloaded_buffer parameter ycm-core/YouCompleteMe#2407

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[READY] Remove unloaded_buffer parameter #542

[READY] Remove unloaded_buffer parameter #542

micbou commented Jul 12, 2016 •

edited by Valloric

Loading

codecov-io commented Jul 12, 2016 •

edited

Loading

puremourning commented Jul 12, 2016

Valloric commented Jul 12, 2016

micbou commented Jul 14, 2016

micbou commented Sep 4, 2016

puremourning commented Sep 4, 2016

ptrv commented Sep 5, 2016

micbou commented Sep 5, 2016

ptrv commented Sep 5, 2016 •

edited

Loading

micbou commented Sep 5, 2016

Qusic commented Sep 6, 2016

micbou commented Oct 24, 2016

Valloric commented Oct 25, 2016

vheon commented Oct 25, 2016

vheon commented Oct 25, 2016

micbou commented Oct 25, 2016

homu commented Oct 25, 2016

homu commented Oct 25, 2016

homu commented Oct 25, 2016

vheon commented Oct 26, 2016

vheon commented Oct 26, 2016

homu commented Oct 26, 2016

homu commented Oct 26, 2016

micbou commented Oct 26, 2016

homu commented Oct 26, 2016

micbou commented Oct 26, 2016

homu commented Oct 26, 2016

homu commented Oct 26, 2016

[READY] Remove unloaded_buffer parameter #542

[READY] Remove unloaded_buffer parameter #542

Conversation

micbou commented Jul 12, 2016 • edited by Valloric Loading

codecov-io commented Jul 12, 2016 • edited Loading

Current coverage is 93.62% (diff: 100%)

puremourning commented Jul 12, 2016

Valloric commented Jul 12, 2016

micbou commented Jul 14, 2016

micbou commented Sep 4, 2016

puremourning commented Sep 4, 2016

ptrv commented Sep 5, 2016

micbou commented Sep 5, 2016

ptrv commented Sep 5, 2016 • edited Loading

micbou commented Sep 5, 2016

Qusic commented Sep 6, 2016

micbou commented Oct 24, 2016

Valloric commented Oct 25, 2016

vheon commented Oct 25, 2016

vheon commented Oct 25, 2016

micbou commented Oct 25, 2016

homu commented Oct 25, 2016

homu commented Oct 25, 2016

homu commented Oct 25, 2016

vheon commented Oct 26, 2016

vheon commented Oct 26, 2016

homu commented Oct 26, 2016

homu commented Oct 26, 2016

micbou commented Oct 26, 2016

homu commented Oct 26, 2016

micbou commented Oct 26, 2016

homu commented Oct 26, 2016

homu commented Oct 26, 2016

micbou commented Jul 12, 2016 •

edited by Valloric

Loading

codecov-io commented Jul 12, 2016 •

edited

Loading

ptrv commented Sep 5, 2016 •

edited

Loading