[READY] Fix errors with Vim versions prior to 7.4.107 when UltiSnips is not loaded

[micbou]

When UltiSnips is not loaded and the Vim version is older than 7.4.107, the following errors:

E117: Unknown function: UltiSnips#SnippetsInCurrentScope
E15: Invalid expression: UltiSnips#SnippetsInCurrentScope( 1 )

will occur each time a buffer is visited. This happens because we rely on a try/except block to catch these errors when evaluating the UltiSnips#SnippetsInCurrentScope function and such blocks are not working prior to Vim 7.4.107. We handle this by checking that the function exists for these versions of Vim.

Fixes #2335.

---

This change is

[puremourning]

nic eone.

---

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

---

python/ycm/youcompleteme.py, line 708 at r1 (raw file):

    # function exists for these versions.
    # TODO: remove this when bumping version requirement to 7.4.107 or greater.
    if ( not vimsupport.VimVersionAtLeast( "7.4.107" ) and

Is there a reason we need this version check. I mean the check in exists should work in any version. That way we can remove the try and let real errors bubble (should they occur). Just a thought

---

Comments from Reviewable

[micbou]

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

---

python/ycm/youcompleteme.py, line 708 at r1 (raw file):

Previously, puremourning (Ben Jackson) wrote…

Is there a reason we need this version check. I mean the check in exists should work in any version. That way we can remove the try and let real errors bubble (should they occur). Just a thought

The version check is here for two reasons: - make it clear that the existence check should be removed when bumping the Vim version; - do the `try/except` block instead of the existence check when possible. I consider that the `try/except` is better because it is supposed to be more efficient (no check each time the function is called) and is not prone to the theoretical issue where the UltiSnips plugin is loaded between the existence check and the function call.

But if you prefer, we could do what you suggest and update the TODO comment by saying that the existence check should be replaced by a try/except block when bumping the Vim version. I don't really mind.

---

Comments from Reviewable

[Valloric]

---

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

---

python/ycm/youcompleteme.py, line 708 at r1 (raw file):

Previously, micbou wrote…

The version check is here for two reasons:

• make it clear that the existence check should be removed when bumping the Vim version;
• do the try/except block instead of the existence check when possible. I consider that the try/except is better because it is supposed to be more efficient (no check each time the function is called) and is not prone to the theoretical issue where the UltiSnips plugin is loaded between the existence check and the function call.

But if you prefer, we could do what you suggest and update the TODO comment by saying that the existence check should be replaced by a try/except block when bumping the Vim version. I don't really mind.

We could also just bump min vim version to 7.4.107. The version in Ubuntu 14.04 (the previous LTS) is older and we drop out-of-the-box support for the previous Ubuntu LTS 6 months after the new LTS comes out. That's about month from now. I'm fine with accelerating this schedule by a month.

I'm also fine with this PR as-is. I'll leave the final call up to you guys.

---

Comments from Reviewable

[micbou]

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

---

python/ycm/youcompleteme.py, line 708 at r1 (raw file):

Previously, Valloric (Val Markovic) wrote…

We could also just bump min vim version to 7.4.107. The version in Ubuntu 14.04 (the previous LTS) is older and we drop out-of-the-box support for the previous Ubuntu LTS 6 months after the new LTS comes out. That's about month from now. I'm fine with accelerating this schedule by a month.

I'm also fine with this PR as-is. I'll leave the final call up to you guys.

I am okay with bumping the Vim version. Let's hear @vheon and @puremourning opinions on that.

---

Comments from Reviewable

[vheon]

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

---

python/ycm/youcompleteme.py, line 708 at r1 (raw file):

Previously, micbou wrote…

I am okay with bumping the Vim version. Let's hear @vheon and @puremourning opinions on that.

Absolutely, let's bump the vim version please ;) Note that we also have #1901 which require a version bump and we need 7.4.143 over there.

---

Comments from Reviewable

[puremourning]

I think changing the min version probably won't solve the problem for a lot of users. In some ways I feel more happy users means less traffic on the tracker, so we might as well just merge the fix :/

On 16 Sep 2016, at 17:40, Andrea Cedraro [email protected] wrote:

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

python/ycm/youcompleteme.py, line 708 at r1 (raw file):
Previously, micbou wrote…

Absolutely, let's bump the vim version please ;) Note that we also have #1901 which require a version bump and we need 7.4.143 over there.
Comments from Reviewable

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

[micbou]

At least, users would know that they need to update their Vim if we bump the version because they would get this message:

YouCompleteMe unavailable: requires Vim 7.4.107+

when loading the plugin. If they still decide to open an issue on the tracker for this then I will be happy to close it 😈

---

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

---

Comments from Reviewable

[puremourning]

Is something blocking merging this? I'm cool with it as-is, tbh.

[micbou]

Closed by PR #1901.