I know @micbou wanted to have a separate request for signature hinting instead of reusing /completions.

Reviewed 4 of 7 files at r1, 14 of 15 files at r2, 7 of 7 files at r3.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

  "min_num_identifier_candidate_chars": 0,
  "semantic_triggers": {},
  "signature_triggers": {},

Is there any reason to expose these options?

ycmd/completers/completer.py, line 187 at r3 (raw file):

    # FIXME: these .get() are required due to the OmniCompleter, which somehow
    # doesn't seem to load the default settings. I guess there is a subset of
    # settings which the Vim client is hard-coded to know about.

All YCM client options are listed in plugin/youcompleteme.py. It was one of the first steps to decouple YCM and ycmd.

ycmd/completers/completer.py, line 193 at r3 (raw file):

              user_options.get( 'signature_triggers' ) or {} ),
            filetype_set = set( self.SupportedFiletypes() ),
            default_triggers = {} )

If I remember correctly LSP server sends the argument hint triggers. Is that right? If so, can ycmd take advantage of that?

ycmd/completers/completer.py, line 240 at r3 (raw file):

      return False

    state = request_data.get( 'signature_help_state', 'INACTIVE' )

Do we need 'ACTIVE' and 'INACTIVE'? Why not just 1 and 0?

ycmd/completers/language_server/language_server_protocol.py, line 258 at r3 (raw file):

          'contentFormat': [
            'plaintext',
            'markdown'

Why markdown? What are we going to do with markdown formatted hover response?

ycmd/completers/language_server/language_server_protocol.py, line 264 at r3 (raw file):

          'signatureInformation': {
            'parameterInformation': {
              'labelOffsetSupport': False, # For now.

Forgive my lazyness, but what is labelOffsetSupport? Or rather what is labelOffset in terms of signature help?

ycmd/completers/language_server/language_server_protocol.py, line 268 at r3 (raw file):

            'documentationFormat': [
              'plaintext',
              'markdown'

Again, what to do with markdown formated response?

Yah I think he's right. This is implementation is convenient but not optimal.

Thanks for taking a look

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

only put it here to ensure it is set in the dict and to be consistent with semantic_triggers. I guess we could remove it or at least not document it (I certainly haven't tested it)

ycmd/completers/completer.py, line 193 at r3 (raw file):

It does, and we do. In language_server_completer.py it gets the signature triggers and calls the method update them with server triggers (just like semantic triggers)

ycmd/completers/completer.py, line 240 at r3 (raw file):

I prefer readable code. 1 and 0 are magic numbers, and don't allow for any tertiary state.

ycmd/completers/language_server/language_server_protocol.py, line 258 at r3 (raw file):

This is expressing a preference order of supported formats. We prefer plaintext, but as markdown is plaintext, too, we can render it and users can still read it.

ycmd/completers/language_server/language_server_protocol.py, line 264 at r3 (raw file):

labelOffsets are kinda dumb. The way the protocol is set up, is you get something like this (without label offsets):

{
    currentParameter: 1,
    currentSignature: 0,
    signature: {
        label: "function_name( arg one, arg& two )" ,
        parameters: [ 
            { label: "arg one" },
            { label" "arg& two" }
        ]
}

So what we do in the YCM client to highlight the current param is:

• for the active parameter, find the string within the signature label
• Use a text property to span that range with the "PMenuSel" highlight group

So in order to find the "range" of the param, you have to use string match.

The purpose of the labelOffset is to say where within the signature's label the parameter actually falls, to (presumably) avoid the string matching, which I suppose could be borked.

It's a really dumb API IMO, but it's simple enough to make it work.

ycmd/completers/language_server/language_server_protocol.py, line 268 at r3 (raw file):

As above. Markdown is plaintext, so we can render it if we have to.

Reviewed 1 of 22 files at r4, 14 of 15 files at r5, 7 of 7 files at r6.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

I'm just worried what will happen if a user starts to mess with these.

ycmd/completers/completer.py, line 240 at r3 (raw file):

Fair enough. Carry on.

Now that there is /signature_help, we should document it.

Reviewed 1 of 12 files at r7, 11 of 11 files at r8.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

README.md, line 105 at r8 (raw file):

for Go (using [Godef][godef] for jumping to definitions), a TSServer-based
completer for JavaScript and TypeScript, a [jdt.ls][jdtls]-based server for
Java, and a [RLS][]-based completer for Rust.  More will be added with time.

I know it's a stupid nitpick, but why two spaces after a full stop?

ycmd/handlers.py, line 130 at r8 (raw file):

  request_data = RequestWrap( request.json )

  if not  _server_state.ShouldUseFiletypeCompleter( request_data ):

if not is followed by two spaces, causing flake8 to fail the build.

Reviewed 13 of 15 files at r10, 4 of 4 files at r11.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/handlers.py, line 130 at r8 (raw file):

Please fix this, so that flake8 stops having fits.

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic and @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

it works the same as the semantic triggers that are already exposed. they are essentially additive. I'm not worried.

ycmd/handlers.py, line 130 at r8 (raw file):

Done.

Reviewed 13 of 15 files at r13, 4 of 4 files at r14.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/default_settings.json, line 7 at r3 (raw file):

Alright, moving on.

The tests are passing, but there's something wrong with codecov. It seems completely busted.

Reviewed 4 of 16 files at r15, 4 of 5 files at r16, 1 of 1 files at r17, 2 of 2 files at r18, 4 of 4 files at r19.
Reviewable status: 0 of 2 LGTMs obtained

Reviewed 8 of 8 files at r20, 7 of 7 files at r21, 8 of 8 files at r22.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)

ycmd/tests/clang/signature_help_test.py, line 38 at r22 (raw file):

@SharedYcmd
def SignatureHelp_NotImplemented_test( app ):

Why is this in clang directory if we don't support signature help in libclang completer?

Reviewable status: 0 of 2 LGTMs obtained (waiting on @bstaletic)

ycmd/tests/clang/signature_help_test.py, line 38 at r22 (raw file):

It's actually testing the behaviour of sending signature help to a completer that doesn't implement it and checking that works. Ultimately I'm toying with ideas about how we communicate that (completer doesn't support sig help) to the ycmd client. But for now, this just covers the code in Completer class which returns the default empty response.

The only issue that I have found is on the client side, so

Reviewable status: 1 of 2 LGTMs obtained

So what is missing at this point? I know you've mentioned that currently YCM/ycmd are wasteful about signature help requests for completers that don't support it, but if we decide to make a /signature_help_available endpoint, we can do that in a separate PR.

Reviewed 2 of 2 files at r23, 9 of 9 files at r24, 1 of 1 files at r25, 1 of 1 files at r26, 1 of 1 files at r27.
Reviewable status: 0 of 2 LGTMs obtained (and 1 stale)

I hit "publish" too fast...

Codecov is complaining about low test coverage. The lines that aren't covered are exceptions and "if not ready, return empty". I'm fine if you decide not to care about it.

Thanks a lot for this PR, it was a massive amount of work.

Reviewable status: 1 of 2 LGTMs obtained

Reviewed 2 of 2 files at r28, 2 of 2 files at r29, 3 of 4 files at r30.
Reviewable status: 1 of 2 LGTMs obtained (waiting on @puremourning)

.gitignore, line 93 at r30 (raw file):

# clangd index data
.clangd/

I though I was the only one being annoyed by this.

ycmd/completers/completer_utils.py, line 144 at r30 (raw file):

    if start_codepoint <= match.end() and match.end() <= column_codepoint:
      return True
  return False

Why is this not returning a codepoint any more?

Reviewable status: 1 of 2 LGTMs obtained (waiting on @bstaletic)

ycmd/completers/completer_utils.py, line 144 at r30 (raw file):

I removed some WIP commits that aren’t used. The idea was to align the popup with the trigger character. I never finished it and we can defer it if we want to investigate later. This just reverts it to the current state.