num_to_str boolean check makes Ultisnips snippet engine laggy

[jsr-p]

Hi

Today I cloned the repository and installed the package directly instead from PyPi.

After reopening nvim I noticed that nvim was very laggy in all python files.
I found out that the issue was with my snippet engine Ultisnips.
Deleting the package and reinstalling it from PyPi made the lag go away.

The version on PyPi is 4.3 and so I went through the git history to check where something significant changed.
I located the issue to be this PR #506.
This PR changes the num_to_str function to include a boolean check.
The helper function is used in the LegacyVim object's eval function here.
Browsing through the Ultisnips source code reveals that it uses vim.eval at many places.
Thus, this change is non-trivial in terms of Ultisnips performance.

I wonder what could be done to circument this issue?

[jsr-p]

Thinking more about, I guess the #506 PR is not compliant with the vim.eval documentation.

The documentation states

vim.eval(str) python-eval
Evaluates the expression str using the vim internal expression
evaluator (see expression). Returns the expression result as:
(1) a string if the Vim expression evaluates to a string or number
(2) a list if the Vim expression evaluates to a Vim list
(3) a dictionary if the Vim expression evaluates to a Vim dictionary Dictionaries and lists are recursively expanded.

In Python, booleans are implemented as a subclass of integers.
In nvim, v:true and v:false evalutes to true and false (boolean values); for boolean values vim uses numbers.
Therefore, by (1) and that booleans are integers in Python, vim.eval('v:true') should evaluate to the Python string "True" and not the boolean True.
And therefore :py3 print(type(vim.eval('v:true'))) should print out <class 'str'> and not <class 'bool'>.
Thus, the extra check in #506 is redundant, right?

[justinmk]

@jsr-p If reverting #506 fixes a performance issue, please send a PR reverting it, with an explanation. I don't think #506 is worth slower performance.

[jsr-p]

@justinmk I have switched to using LuaSnip but thanks for considering my comment on the performance!

[wookayin]

@justinmk I think this might need to be revisited around 0.5.0 release because this could be a breaking change and have some performance impacts. (technically speaking, #506 is a breaking change on the behavior)

[justinmk]

No objection to reverting #506 . It wasn't given a strong justification