refactor(char_classing): use inbuilt vim functions

[josh-nz]

This closes #50.

This took me quite a while to follow through. In short, it's hitting the while loop one additional time because of the utils.char_class falling through to the default value of other which is the same class as the initial match. So the resulting offset is one less than it should be, resulting in a column of zero instead of the correct 1.

The docs for vim.fn.strcharpart say it returns empty string on error, which is what is happening here when the index is outside the line. It made the most sense to add a guard for this edge case so I introduced an additional char_classes enum member.

At least, this is how it was working until the simultaneous change to use the FFI in #49. The tests still pass, things still look to work, my issue remains fixed, but this might want a slightly more detailed review due to the FFI changes.

Edit: I also added a missed test for the emoji char_classes member from #49, in commit bf4df53

[willothy]

Awesome, thanks! Sorry, I maybe should've waited to merge the FFI branch.

[josh-nz]

No worries, I didn't post the bug until after I had a solution. I should have posted it sooner and mentioned I was looking into it.

I did test the FFI changes on their own without the fix here, and they didn't solve this issue alone.

[willothy]

Nice, this looks good to me! I'll let @tris203 look over it before we merge though :)

[tris203]

I'd have to look at the other code

But can we fix at the other side with a <= or something?

The char class function is basically just the cls function from Nvim/Vim and id like to keep it as close to that as possible

My preference would be to fix the prev_word implementation to exit early and add an assert in char_class that the string isnt empty

If this is possible

https://github.com/neovim/neovim/blob/master/src%2Fnvim%2Ftextobject.c#L273-L306

On my phone, so apologies for formatting

[josh-nz]

I'll take a look, should be able to move the empty string check I think. I wasn't aware of the Vim cls stuff.

[josh-nz]

The above changes appear to work for the issue I logged, and all tests pass.

If I add assert(char ~= "") to the char_class function, a number of tests involving whitespace and padding fail. I'm not sure if this is because they were originally passing incorrectly and this change has revealed a source of issues, or if this assertion has legitimately broken things.

Currently this assertion has not been committed.

[josh-nz]

Upon initial investigation, it looks like there are a number of additional places that would need to handle the empty string case since it is no longer part of the char_class function; every call to this function would need to be guarded against empty string. It looks like there 13 or so calls to this function (outside of tests), which really adds a lot of extra logic everywhere.

I wonder if it's best to disregard adding an empty string assertion in char_class since things look to be working ok without it. It might be more effort to support it than is worthwhile.

[tris203]

I don't know if we need the guard and it might be that the offset ranges in the while loop need to be modified to be non inclusive

[tris203]

I think we need to make emojis return 3 for this to be complete,

as in this example the b is under the p not the cloud

large change

[tris203]

this turned into a bigger refactor of how we class chars but this should have much better support for different langs and locales

ffi was short lived, as there is a vim function for it directly

@willothy any thoughts?
@josh-nz thanks for raising the right questions for this!

[willothy]

Just these (other isn't used anywhere anymore), then looks good to me!

[willothy]

lgtm! sorry for confusion