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

Convert insertion_text field to typed_text and completion_chunks #664

Closed
wants to merge 1 commit into from
Closed

Conversation

Qusic
Copy link

@Qusic Qusic commented Dec 5, 2016

insertion_text is removed in favor of typed_text and completion_chunks.

typed_text: 'fun',
completion_chunks: [
  {text: 'fun(', placeholder: false},
  {text: 'int x', placeholder: true},
  {text: ')', placeholder: false}
]

typed_text is inspired by clang. It is the string that the user is expected to type to match exactly this completion suggestion, which can be used for filtering, sorting and also grouping when function overloads are represented by separate completion suggestions.

In BuildCompletionData, typed_text is the only required parameter, completion_chunks defaults to [{text: typed_text, placeholder: false}], for the sake of convenience.


This change is Reviewable

@codecov-io
Copy link

codecov-io commented Dec 5, 2016

Codecov Report

Merging #664 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #664      +/-   ##
==========================================
+ Coverage   94.84%   94.85%   +<.01%     
==========================================
  Files          79       79              
  Lines        5297     5302       +5     
  Branches      171      171              
==========================================
+ Hits         5024     5029       +5     
  Misses        228      228              
  Partials       45       45

@vheon
Copy link
Contributor

vheon commented Dec 5, 2016

I'm not sure if we talked about this in the past but is the placeholder really necessary? To me seems redundant: consider a function

void foo( int a, int b, int c, int d, int e );

a completion request for such function would have the completion_chunks field like:

 [
  {"tex": "foo(", "placeholder": false},
  {"tex": "int a", "placeholder": true},
  {"text": "int b", "placeholder": true},
  {"text": "int c", "placeholder": true},
  {"text": "int d", "placeholder": true},
  {"text": "int e", "placeholder": true},
  {"text": ")", "placeholder": false}
]

couldn't we simplify this to:

 {
  "start": "foo(",
   "end": ")",
  "parameters": [ "int a", "int b", "int c", "int d", "int e" ]
}

??

Do you see any drawback doing it like this? Anything that will prevent us in the future to do something?

@Qusic
Copy link
Author

Qusic commented Dec 5, 2016

The syntax varies with languages.
[objcObject firstArg:arg1 secondArg:arg2]

@Valloric
Copy link
Member

@vheon If I understand what @Qusic is saying, the "placeholder" concept comes directly from libclang. And we can't just send a list of params because as his example shows, that doesn't work right with objc. Also, having the placeholder flag does not preclude us from having explicit parameters in the future.

I'm fine with this API, though I haven't looked at the code yet (let's get agreement on the API first).

@micbou @puremourning Thoughts?

@richard1122
Copy link
Contributor

Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@richard1122
Copy link
Contributor

This API looks good right now. We can do small changes later when face any limitations for other languages.

@richard1122
Copy link
Contributor

:lgtm:


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


Comments from Reviewable

@richard1122
Copy link
Contributor

@micbou @puremourning Is this OK with this API ?

@zzbot
Copy link
Contributor

zzbot commented Oct 9, 2017

☔ The latest upstream changes (presumably #843) made this pull request unmergeable. Please resolve the merge conflicts.

@Valloric
Copy link
Member

Closing PR as its not moving forward.

@Valloric Valloric closed this Nov 15, 2018
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.

6 participants