[READY] Relax semantic trigger matching to match between column start and current cursor column

[micbou]

This PR fixes issues ycm-core/YouCompleteMe#1602 and ycm-core/YouCompleteMe#1930.

I updated the tests and added some cases.

[puremourning]

Generally looks good.

Some people thoughts:

• does this change potentially break any existing custom triggers? I know there are a number of hacks out there to (heinously?) trigger semantic on each keystroke.
• should we add tests for the cases where this came up like triggers including identifier chars?
• how does this change the interaction with the cache (if at all)? I confess I can't remember what goes in as the key in the cache, but it seems now that the 'prefix' is more fluid which might interfere with existing assumptions.

---

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

---

ycmd/completers/completer_utils.py, line 87 [r1] (raw file):
I guess this could benefit from a comment.

---

Comments from the review on Reviewable.io

[micbou]

does this change potentially break any existing custom triggers? I know there are a number of hacks out there to (heinously?) trigger semantic on each keystroke.

Lookahead regex hack will work again (it was broken by PR #292) but will not be needed anymore. I can't think of any other hack.

should we add tests for the cases where this came up like triggers including identifier chars?

Probably. We could use our dummy completer for these tests.

how does this change the interaction with the cache (if at all)? I confess I can't remember what goes in as the key in the cache, but it seems now that the 'prefix' is more fluid which might interfere with existing assumptions.

This PR only affects the value returned by MatchesForFiletype and this value is not cached.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

ycmd/completers/completer_utils.py, line 87 [r1] (raw file):
I'll add one.

---

Comments from the review on Reviewable.io

[puremourning]

OK with that minor comment this . Great work.

I wonder if there is anywhere that this behaviour is currently documented, and if we need to update that? The previous behaviour was fairly subtle, and now it is subtly different. Not really sure. Maybe an update to YCM's README for the option, or the comments around the default trigger map.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[vheon]

I'm a little puzzled by some changes (I have to admit that I don't remember how this whole piece of code works):

• Why we need both start_column and column_num? isn't column_num enough to determine if we have to trigger a semantic completion?
• Why are we checking for match.end() <= while before we were checking if match.end() ==?

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[oblitum]

Once remaining comments are added, it's for me too.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from the review on Reviewable.io

[micbou]

I added the comment (hope it is understandable) and an integration test.

Why we need both start_column and column_num? isn't column_num enough to determine if we have to trigger a semantic completion?

Why are we checking for match.end() <= while before we were checking if match.end() ==?

Current behavior is to check at start_column because this is where we expect to find a semantic (non-identifier) trigger. However, this doesn't work when the trigger is an identifier itself (more precisely, when it ends with an identifier character). For this reason, we need to match between start_column and column_num. An example:

foo.bar_qux
    ^      ^
    |      column_num
    start_column

If our trigger is ., its end will match at 4 = start_column (0-based index). If our trigger is _, its end will match at 9 <= column_num.

---

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

---

ycmd/completers/completer_utils.py, line 87 [r1] (raw file):
Done.

---

Comments from the review on Reviewable.io

[oblitum]

@homu +r. Good explanation, I'm not sure the act of triggering needed to be so tricky!

---

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

---

Comments from the review on Reviewable.io

[oblitum]

@homu r+

---

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

---

Comments from the review on Reviewable.io

[homu]

📌 Commit 0af311a has been approved by oblitum

[homu]

⌛ Testing commit 0af311a with merge f1c4fb3...

[homu]

☀️ Test successful - status