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] Improve Clangd completer initialization #1187
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1187 +/- ##
=========================================
+ Coverage 97.28% 97.38% +0.1%
=========================================
Files 95 95
Lines 7102 7529 +427
=========================================
+ Hits 6909 7332 +423
- Misses 193 197 +4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @micbou)
ycmd/completers/cpp/clangd_completer.py, line 133 at r1 (raw file):
return None, None if not clangd:
Why not just else:
?
This brings the following changes to the Clangd completer: - do not raise an exception when initializing the Clangd completer since this prevents ycmd from falling back to the libclang completer; - never fall back to the third-party Clangd if the one specified in 'clangd_binary_path' is not valid; - expand environment variables and '~' in the path specified by the 'clangd_binary_path' option; - do not print in the logs that the Clangd library is outdated if the path does not point to an executable; - cosmetic changes like capitalizing Clangd in comments.
1bbc404
to
044880c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)
ycmd/completers/cpp/clangd_completer.py, line 133 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Why not just
else:
?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 LGTMs obtained
Thanks for the PR! @zzbot r+ |
📌 Commit 044880c has been approved by |
[READY] Improve Clangd completer initialization This PR brings the following changes to the Clangd completer: - do not raise an exception when initializing the Clangd completer since this prevents ycmd from falling back to the libclang completer; - never fall back to the third-party Clangd if the one specified in `clangd_binary_path` is not valid. The reason behind that change is that I think most users are going to be confused by such fallback since they'll think that the Clangd from `clangd_binary_path` is used while in fact it's not. I can revert this change if you disagree; - expand environment variables and `~` in the path specified by the `clangd_binary_path` option; - do not print in the logs that the Clangd library is outdated if the path does not point to an executable; - cosmetic changes like capitalizing Clangd in comments and logs. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/ycmd/1187) <!-- Reviewable:end -->
💔 Test failed - status-travis |
☀️ Test successful - status-appveyor, status-travis |
[READY] Update ycmd Include the following changes: - PR ycm-core/ycmd#1180: trigger semantic completion when instructed by server; - PR ycm-core/ycmd#1184: simplify LSP completer API for starting server; - PR ycm-core/ycmd#1187: improve Clangd completer initialization; - PR ycm-core/ycmd#1191: ease Clangd completer initialization; - PR ycm-core/ycmd#1193: fix system header search paths on macOS for Objective-C++; - PR ycm-core/ycmd#1194: update Jedi to 0.13.3 and Parso to 0.3.4; - PR ycm-core/ycmd#1195: ignore identifiers returned by TSServer in JavaScript; - PR ycm-core/ycmd#1196: update TSServer to 3.3.3333; - PR ycm-core/ycmd#1197: support -B flag in C-family languages. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/valloric/youcompleteme/3339) <!-- Reviewable:end -->
This PR brings the following changes to the Clangd completer:
clangd_binary_path
is not valid. The reason behind that change is that I think most users are going to be confused by such fallback since they'll think that the Clangd fromclangd_binary_path
is used while in fact it's not. I can revert this change if you disagree;~
in the path specified by theclangd_binary_path
option;This change is