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] Update ycmd #2768

Merged
merged 4 commits into from
Sep 10, 2017
Merged

[READY] Update ycmd #2768

merged 4 commits into from
Sep 10, 2017

Conversation

micbou
Copy link
Collaborator

@micbou micbou commented Sep 10, 2017

This new version of ycmd includes the following changes:

The g:ycm_max_num_candidates and g:ycm_max_num_identifier_candidates options are added to the documentation.

The link to ycmd extra conf is updated.

Fixes #2562.


This change is Reviewable

@micbou micbou changed the title [READY] Update ycmd [WIP Update ycmd Sep 10, 2017
@micbou micbou changed the title [WIP Update ycmd [WIP] Update ycmd Sep 10, 2017
@puremourning
Copy link
Member

Thanks. Big upgrade :)


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.


README.md, line 1747 at r1 (raw file):

### The `g:ycm_max_num_candidates` option

This option controls the maximum number of candidates other than identifiers

Instead of "candidates other than identifiers". How about "The maximum number of semantic completion suggestions" ?

I don't think candidates is a term users are going to be immediately familiar with, but "suggestions" feels more natural. Also, non-identifier candidates are "semantic completion candidates", which is a little clearer.


README.md, line 1748 at r1 (raw file):

This option controls the maximum number of candidates other than identifiers
shown in the completion menu. Use the `g:ycm_max_identifier_candidates` option

Maybe "Note, this only applies to semantic completion suggestions; see g:ycm_max_identifier_candidates to limit the number of suggestions from the liknkidentifier engine*/link*` ?


README.md, line 1753 at r1 (raw file):

A special value of `0` means there is no limit.

**NOTE:** Setting this option to `0` or to a value too high is not recommended

Rather than "too high", could we put a limit on it ? E.g. "Setting this value to 0 or to a value greater than 100..."

Or perhaps, just say "The default value is correct for most users, and should not be changed. For advanced users, it is not recommended to set this to 0 or a value greater than 100" (or some reasonable maximum)


README.md, line 1754 at r1 (raw file):

**NOTE:** Setting this option to `0` or to a value too high is not recommended
as it will slow down completion when there are too much candidates.

'Too much' -> 'a very large number of' ('too many' would be a direct correction, but it implies a limit we haven't published)


README.md, line 1762 at r1 (raw file):

The g:ycm_max_num_identifier_candidates option

Same general comments as above. Some ideas about how to rephrase to be a little clearer.


Comments from Reviewable

@codecov-io
Copy link

codecov-io commented Sep 10, 2017

Codecov Report

Merging #2768 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2768   +/-   ##
=======================================
  Coverage   91.07%   91.07%           
=======================================
  Files          20       20           
  Lines        1939     1939           
=======================================
  Hits         1766     1766           
  Misses        173      173

@bstaletic
Copy link
Collaborator

bstaletic commented Sep 10, 2017

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


README.md, line 1747 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Instead of "candidates other than identifiers". How about "The maximum number of semantic completion suggestions" ?

I don't think candidates is a term users are going to be immediately familiar with, but "suggestions" feels more natural. Also, non-identifier candidates are "semantic completion candidates", which is a little clearer.

@micbou seens like he's been insisting on calling it "non-identifier candidates". It confused me as well at first, so I wonder if there was a reason. It's definitely not clear enough for the end user.


Comments from Reviewable

@micbou micbou force-pushed the update-ycmd branch 3 times, most recently from 4726674 to 965f918 Compare September 10, 2017 11:17
@micbou
Copy link
Collaborator Author

micbou commented Sep 10, 2017

Changed OmniCompleter_GetCompletions_Cache_List_Unicode_test to a test that's expected to fail since we now filter and sort candidates when the query is empty (see PR ycm-core/ycmd#819).

@puremourning Thanks for the suggestions.


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


README.md, line 1747 at r1 (raw file):
Yes, "suggestions" is better than "candidates".

@micbou seens like he's been insisting on valling it "non-identifier candidates". It confused me as well at first, so I wonder if there was a reason. It's definitely not clear enough for the end user.

I didn't use "semantic candidates" because of the filename completer but it can also be considered as a semantic engine after all.


README.md, line 1748 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Maybe "Note, this only applies to semantic completion suggestions; see g:ycm_max_identifier_candidates to limit the number of suggestions from the liknkidentifier engine*/link*` ?

I rephrased "semantic completion suggestions" to avoid a repetition with the previous sentence.


README.md, line 1753 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Rather than "too high", could we put a limit on it ? E.g. "Setting this value to 0 or to a value greater than 100..."

Or perhaps, just say "The default value is correct for most users, and should not be changed. For advanced users, it is not recommended to set this to 0 or a value greater than 100" (or some reasonable maximum)

I went for 100.


README.md, line 1754 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

'Too much' -> 'a very large number of' ('too many' would be a direct correction, but it implies a limit we haven't published)

Done.


README.md, line 1762 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Same general comments as above. Some ideas about how to rephrase to be a little clearer.

Done.


Comments from Reviewable

@micbou micbou changed the title [WIP] Update ycmd [READY] Update ycmd Sep 10, 2017
@bstaletic
Copy link
Collaborator

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


README.md, line 1747 at r1 (raw file):

Previously, micbou wrote…

Yes, "suggestions" is better than "candidates".

@micbou seens like he's been insisting on valling it "non-identifier candidates". It confused me as well at first, so I wonder if there was a reason. It's definitely not clear enough for the end user.

I didn't use "semantic candidates" because of the filename completer but it can also be considered as a semantic engine after all.

Minor comment, feel free to disregard.

Maybe we should mention both "semantic and filename completion suggestions", but just not call it "non-identifier candidates".


Comments from Reviewable

@puremourning
Copy link
Member

:lgtm: awesome, thanks!


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


README.md, line 1747 at r1 (raw file):

Previously, bstaletic (Boris Staletic) wrote…

Minor comment, feel free to disregard.

Maybe we should mention both "semantic and filename completion suggestions", but just not call it "non-identifier candidates".

I think we can justifiably say file path (and snippet?) completion is semantic for these purposes :)


Comments from Reviewable

@bstaletic
Copy link
Collaborator

:lgtm:

@zzbot r=puremourning


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


README.md, line 1747 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

I think we can justifiably say file path (and snippet?) completion is semantic for these purposes :)

Alright, I'm fine with that.


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

📌 Commit 1965d3e has been approved by puremourning

@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

⌛ Testing commit 1965d3e with merge fd84aba...

zzbot added a commit that referenced this pull request Sep 10, 2017
[READY] Update ycmd

This new version of ycmd includes the following changes:

 - PR ycm-core/ycmd#795: add option to make relative paths in flags from extra conf absolute;
 - PR ycm-core/ycmd#802: fix compilation on Haiku;
 - PR ycm-core/ycmd#804: add libclang detection on FreeBSD;
 - PR ycm-core/ycmd#808: write python used during build before installing completers;
 - PR ycm-core/ycmd#810: support unknown languages from tags;
 - PR ycm-core/ycmd#811: update Universal Ctags languages list;
 - PR ycm-core/ycmd#814: resolve symlinks in extra conf glob patterns;
 - PR ycm-core/ycmd#815: update JediHTTP;
 - PR ycm-core/ycmd#816: update Boost to 1.65.0;
 - PR ycm-core/ycmd#819: filter and sort candidates when query is empty;
 - PR ycm-core/ycmd#820: improve LLVM root path search for prebuilt binaries;
 - PR ycm-core/ycmd#822: inline critical utility functions;
 - PR ycm-core/ycmd#824: do not sort header paths in filename completer;
 - PR ycm-core/ycmd#825: implement partial sorting;
 - PR ycm-core/ycmd#830: add max_num_candidates option;
 - PR ycm-core/ycmd#831: fix multiline comments and strings issues;
 - PR ycm-core/ycmd#832: update Clang to 5.0.0.

The `g:ycm_max_num_candidates` and `g:ycm_max_num_identifier_candidates` options are added to the documentation.

The link to ycmd extra conf is updated.

Fixes #2562.

<!-- 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/2768)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: puremourning
Pushing fd84aba to master...

@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@bstaletic
Copy link
Collaborator

bstaletic commented Sep 10, 2017 via email

Include following changes:
 - add option to make relative paths in flags from extra
   conf absolute;
 - fix compilation on Haiku;
 - add libclang detection on FreeBSD;
 - write python used during build before installing
   completers;
 - support unknown languages from tags;
 - update Universal Ctags languages list;
 - resolve symlinks in extra conf glob patterns;
 - update JediHTTP;
 - update Boost to 1.65.0;
 - filter and sort candidates when query is empty;
 - improve LLVM root path search for prebuilt binaries;
 - inline critical utility functions;
 - do not sort header paths in filename completer;
 - implement partial sorting;
 - add max_num_candidates option;
 - fix multiline comments and strings issues;
 - update Clang to 5.0.0.
This test is expected to fail since we now filter and sort candidates on empty
query.
@micbou
Copy link
Collaborator Author

micbou commented Sep 10, 2017

It probably didn't like the direct merge of PR #2770.

@zzbot r+


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


Comments from Reviewable

@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

📌 Commit c5bec8f has been approved by micbou

@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

⌛ Testing commit c5bec8f with merge 9ca755a...

zzbot added a commit that referenced this pull request Sep 10, 2017
[READY] Update ycmd

This new version of ycmd includes the following changes:

 - PR ycm-core/ycmd#795: add option to make relative paths in flags from extra conf absolute;
 - PR ycm-core/ycmd#802: fix compilation on Haiku;
 - PR ycm-core/ycmd#804: add libclang detection on FreeBSD;
 - PR ycm-core/ycmd#808: write python used during build before installing completers;
 - PR ycm-core/ycmd#810: support unknown languages from tags;
 - PR ycm-core/ycmd#811: update Universal Ctags languages list;
 - PR ycm-core/ycmd#814: resolve symlinks in extra conf glob patterns;
 - PR ycm-core/ycmd#815: update JediHTTP;
 - PR ycm-core/ycmd#816: update Boost to 1.65.0;
 - PR ycm-core/ycmd#819: filter and sort candidates when query is empty;
 - PR ycm-core/ycmd#820: improve LLVM root path search for prebuilt binaries;
 - PR ycm-core/ycmd#822: inline critical utility functions;
 - PR ycm-core/ycmd#824: do not sort header paths in filename completer;
 - PR ycm-core/ycmd#825: implement partial sorting;
 - PR ycm-core/ycmd#830: add max_num_candidates option;
 - PR ycm-core/ycmd#831: fix multiline comments and strings issues;
 - PR ycm-core/ycmd#832: update Clang to 5.0.0.

The `g:ycm_max_num_candidates` and `g:ycm_max_num_identifier_candidates` options are added to the documentation.

The link to ycmd extra conf is updated.

Fixes #2562.

<!-- 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/2768)
<!-- Reviewable:end -->
@zzbot
Copy link
Contributor

zzbot commented Sep 10, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: micbou
Pushing 9ca755a to master...

@zzbot zzbot merged commit c5bec8f into ycm-core:master Sep 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

YouCompleteMe works great for C/C++. Not so great for python (socket library)
5 participants