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.

Sign up for GitHub

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

Jump to bottom

[READY] Add error messages when ycmd crashed #2142

Merged
merged 2 commits into from May 3, 2016
Merged

[READY] Add error messages when ycmd crashed #2142

merged 2 commits into from May 3, 2016

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Apr 26, 2016 •
edited by Valloric

See PR ycm-core/ycmd#467.

This change is Reviewable

@Valloric
Copy link
Member

:lgtm:

This shit is the bee's knees! :D

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

python/ycm/youcompleteme.py, line 93 [r1] (raw file):
Oh this is fantastic!

python/ycm/youcompleteme.py, line 188 [r1] (raw file):
Holy crap this is seriously fantastic! Not only does it detect when the log file is not there, it tells the user what to toggle to get the logs.

👏 👏 👏

Comments from Reviewable

@micbou micbou changed the title [RFC] Add error messages when ycmd crashed [WIP] Add error messages when ycmd crashed Apr 27, 2016
@micbou micbou force-pushed the check-core branch 4 times, most recently from 7bb7857 to 3ed71b8 Compare April 27, 2016 19:03
@micbou
Copy link
Collaborator Author

micbou commented Apr 27, 2016

This PR now uses the symbolic names for exit statuses defined in ycm-core/ycmd#467. I made some simplifications too.

It is really a shame that there is no switch blocks in Python. I find them neater than elif ... blocks.

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

Comments from Reviewable

@puremourning
Copy link
Member

I hear that dictionaries are the new switch statement. More pythonic madness.

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

Comments from Reviewable

@Valloric
Copy link
Member

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

Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Apr 27, 2016

So to recap the potential benefit of this PR:

  • 1 less waitpid
  • less vimscript more python
  • improved error messages

What can I say? :lgtm:

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

Comments from Reviewable

@vheon
Copy link
Contributor

vheon commented Apr 27, 2016

I have a strange problem with reviewable: if I trigger homu from here I get an error and I cannot publish the message 😕

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

Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented Apr 27, 2016

I encountered some network issues with reviewable this evening but not this kind of bug. Anyway, don't trigger homu until ycm-core/ycmd#467 is merged.

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

Comments from Reviewable

@micbou
Add error messages when ycmd crashed
b440f68 
Display an error message to the user depending on the status code
returned by the ycmd server.
Remove ycm_core checks in plugin/youcompleteme.vim. These checks are
now done by the ycmd server.
Do not start a separate process to check the core version but rely on
ycmd returning a specific exit code. This slightly improves the Vim
startup time.
@micbou
Copy link
Collaborator Author

micbou commented May 2, 2016

Updated to latest ycmd so that it includes PR ycm-core/ycmd#467. Changing the tag to READY.

Reviewed 2 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@micbou micbou changed the title [WIP] Add error messages when ycmd crashed [READY] Add error messages when ycmd crashed May 2, 2016
@vheon
Copy link
Contributor

vheon commented May 2, 2016

@micbou I believe you have all the approval you need. If is all set I think you can fire up homu yourself 👍

Reviewed 2 of 2 files at r5, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@micbou
Copy link
Collaborator Author

micbou commented May 3, 2016

@homu r=vheon

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

Comments from Reviewable

@homu
Copy link
Contributor

homu commented May 3, 2016

📌 Commit d2beb20 has been approved by vheon

@homu homu merged commit d2beb20 into ycm-core:master May 3, 2016
homu added a commit that referenced this pull request May 3, 2016
@homu
Auto merge of #2142 - micbou:check-core, r=vheon
87f0f7e 
[READY] Add error messages when ycmd crashed

See PR ycm-core/ycmd#467.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/2142)
<!-- Reviewable:end -->
@homu
Copy link
Contributor

homu commented May 3, 2016

⚡ Test exempted - status

@micbou micbou deleted the check-core branch May 3, 2016 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@micbou @Valloric @puremourning @vheon @homu