Improves semantic trigger match.

[oblitum]

Improves semantic trigger match.

Match triggers until user's caret column solely. Trying to match
triggers with characters after user's caret column doesn't seem
to have any utility.

---

This is the first in a series of pull-requests that will transparently enable
#1300 for any completer that cares for. After this minimal foundation is

stablished, an initial Swift completer will be available, supporting diagnostics,
fix-it's, semantic completion and argument hints. Changes will also be
proposed to enable hints for C and C++.

[puremourning]

I think this change should come with some tests to go with it, and a code comment explaining the behaviour. It's not super-obvious what's going on, and/or why this is necessary 😬

---

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

---

Comments from the review on Reviewable.io

[puremourning]

This is the first in a series of pull-requests that will transparently enable
#1300 for any completer that cares for. After this minimal foundation is
stablished, an initial Swift completer will be available, supporting diagnostics,
fix-it's, semantic completion and argument hints. Changes will also be
proposed to enable hints for C and C++.

BTW. This is very, very cool. I'm super excited! Like, it's Christmas or something.

---

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

---

Comments from the review on Reviewable.io

[Valloric]

I does make sense to only try to match until the user's caret column. :)

Like @puremourning said, having some tests here would be nice.

---

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

---

Comments from the review on Reviewable.io

[oblitum]

Reworded commit message and added comment. Will try to figure out a test tomorrow (too late here).

see ya

---

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

---

Comments from the review on Reviewable.io

[oblitum]

Guys... I just gave up coming up with a test for this. It's clear it makes sense, adds a performance improvement, don't break tests, but I just forgot how it changes behavior, if it ever does, with the current state of the project.

At the time I applied it I was trying several kinds of mechanisms for argument hints and in one of those I needed that, not sure whether it's still necessary.

---

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

---

Comments from the review on Reviewable.io

[puremourning]

IIRC it was to do with ignoring whitespace after a trigger? e.g. trigger on , but also trigger when the text is ignore ,<space><space><space> etc ?
Maybe the test only makes sense with that situation? I think I remember that you responded to another issue on the tracker pointing at something related

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[oblitum]

Exactly, and I just expended quite a lot of time trying to find that issue where I commented about that, but I just coudn't find it.

---

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

---

Comments from the review on Reviewable.io

[oblitum]

Regarding it being useful about ignoring whitespace after a trigger, I just don't recall :-(

---

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

---

Comments from the review on Reviewable.io

[puremourning]

Related? ycm-core/YouCompleteMe#1337

On 3 Jan 2016, at 17:04, Francisco Lopes notifications@github.com wrote:

Regarding it being useful about ignoring whitespace after a trigger, I just don't recall :-(

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

Comments from the review on Reviewable.io https://reviewable.io/reviews/valloric/ycmd/292#-:-K77QoVNBiJTbPyeK5yU:1796791813
—
Reply to this email directly or view it on GitHub #292 (comment).

[oblitum]

Hmm, actually no, although this pull contains parts I'll pull again later. I commented about this specifically with someone at the issue tracker, he was having doubts regarding how it was actually working since the code was looking for matches at the entire line, I guess for Obj-C completion. I suggested this could be a fix for his problems.

---

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

---

Comments from the review on Reviewable.io

[Valloric]

Let's confirm that this change is useful in some way before committing it. And if we do want to commit it, we don't necessarily need a test proving a bug is fixed, we can go with a test that proves chars after the caret can't be examined.

Also, it's one simple line change with no conditionals/loops that's exercised by every other completion test we have, so I'd be fine with it landing without a test.

---

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

---

Comments from the review on Reviewable.io

[vheon]

If we decide to commit this I think that we can simplify some code, for example the _StringTriggerMatches function can be simply replaced with line_value.endswith( trigger )

---

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

---

Comments from the review on Reviewable.io

[oblitum]

@vheon Actually I'll propose remotion of _StringTriggerMatches. It's being used solely in tests, it's not being used by completers because all triggers when prepared are transformed into regexes. The current state of the code base is a bit confuse regarding that because it appear string matches are being used while in truth they are not.

---

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

---

Comments from the review on Reviewable.io

[vheon]

Yes, this is very confusing.

Anyway IMHO a test like:

  ok_( not cu._MatchesSemanticTrigger( 'foo->bar', 4, ['->'] ) )

will be enough for this change.

---

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

---

Comments from the review on Reviewable.io

[oblitum]

@vheon It seems like MatchesSemanticTrigger_Basic_test already covers that.

I changed my commit description, I think this is nothing beyond that, a minor optimization I've stumbled upon. Even though minor, I just happen not to like regex matching at will :)

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from the review on Reviewable.io

[vheon]

@vheon It seems like MatchesSemanticTrigger_Basic_test already covers that.

I changed my commit description, I think this is nothing beyond that, a minor optimization I've stumbled upon. Even though minor, I just happen not to like regex matching at will :)

If tests already cover this, then I'm ok with merging this as it is

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from the review on Reviewable.io

[Valloric]

OK, let's merge this then. Thanks for the PR!

@homu r+

[homu]

📌 Commit c26a4e5 has been approved by Valloric

[homu]

⌛ Testing commit c26a4e5 with merge 9e0c19d...

[homu]

☀️ Test successful - status

[oblitum]

For record, there's more room for improvement over regex matching. After taking a look at it again, initially it was regex matching the entire line regardless of start_column, this is now fixed, but regex matching from beginning of line until trigger's ending position, for which we already have its termination through start_column, that was already used to cut the line... meaning, the proper regex should be trigger+$ without need for loop if match.end() == start_column.

Taking the current code aside a bit, and thinking of the simplest form of trigger matching to its core, when we already have its termination (start_column):

1. cut the line at start_column.
2. match trigger+$ with the line. == no need to range a line regex matching

I know perf diff should be minor, but besides this, I guess code would get simpler and of clearer intent.

[vheon]

meaning, the proper regex should be trigger+$ without need for loop if match.end() == start_column.

I thought of this when I saw the code for the code review. I'm not super familiar with that part of the codebase so I don't know how complex it would be to put this in place.

[oblitum]

meaning, the proper regex should be trigger+$ without need for loop if match.end() == start_column.

I thought of this when I saw the code for the code review. I'm not super familiar with that part of the codebase so I don't know how complex it would be to put this in place.

Nor do I, although at its core it's a simplification, I'm not sure what the extent of refactoring for compiled trigger+$ use could be. I was taking a second look at it because I wanted to have access to triggers from client side, for example, this is what I really need here https://github.com/Valloric/YouCompleteMe/pull/1902/files#diff-414ad007afc1163aea017b394fe82f27R49 instead of IsIdentifier(). It seems to me that whitespace and trigger checking could supplant the need for IsIdentifier() in general. I dunno how many forms are in use for boundary checks related to trigger termination etc, sadly from client side there's only the "IsIdentifier()" util, for simple usage.