:TOhtml will be subject to breaking changes

[laniakea64]

As part of looking into #85 I ran our syntax highlighting tests against the latest Neovim development pre-release (NVIM v0.10.0-dev-2849+g4946489e2). It went badly:

1. The strings our test runner expects in the HTML were not present, so it panicked.

2. After fixing (1), 100% of our test cases failed. This appears to be due to changes in :TOhtml:

• Some internal groups, particularly ones intentionally not linked to any highlighting type (such as justRecipeParenDefault and justShellSetValue), are now appearing in the html. These are implementation details that should not be required to match to pass the tests.
• Tabs are being converted to spaces. The integrity of the tests requires keeping tabs as tabs and spaces as spaces.
• Trailing newline handling has changed.

That's only what I noticed, might have missed other differences.

Not sure we need to do anything at this stage, since this is pre-release and our tests pass with latest Neovim release 0.9.5, but thought this should be noted.

[NoahTheDuke]

Why do the tests require tabs? The rest seems like it should be surmountable.

[laniakea64]

1. just generally does not like recipe body indentation that mixes tabs and spaces, but there are a few cases where it is valid. Our test suite includes tab indents, space indents, valid and invalid mixes of tabs and spaces. We verify that invalid mixes are highlighted as Error while valid indentation is not.

2. In some contexts, just accepts any whitespace, including tabs, so some test cases contain tabs in unusual places to ensure it is accepted.

Coalescing everything to spaces will make these kinds of tests unsound, unavoidably adding room for incorrect passes or failures.

[laniakea64]

The breaking changes are here to stay.

From https://neovim.io/doc/user/vim_diff.html#nvim-removed -

The old :TOhtml, replaced by a Lua version (contains many differences)

neovim/neovim@2f85bbe

Looks like we'll need to come up with something else in order to sustain Neovim support in the long term. In fact, looking at the context of that Neovim change and looking under the hood of Vim's :TOhtml...are we really sure it's wise to have :TOhtml the backbone of a test suite? 😱

After #84 is merged we can investigate options here.

[laniakea64]

After #84 is merged we can investigate options here.

I take that back. Instead, let's add fix for this into that PR.

Specifically, think we can fix this by our own implementation of conversion to HTML. Trying this out, ~70 lines of Vim script does the job for us, MUCH less complex than :TOhtml. And discovered that :TOhtml output is actually wrong for some of our test cases: it strips some trailing whitespace, which in our case is incorrect.

Also, the custom Vim script so far is almost 2x faster than :TOhtml in benchmarks. But this isn't final, this version is not ready to push yet, works in Vim but not yet working in Neovim.

[NoahTheDuke]

Holy shit. That's great news. Please post whatever you write so I can learn from it, I'm very poor at vimscript. (Maybe it could even be in lua and be faster still.)

[laniakea64]

Please post whatever you write so I can learn from it,

Done! It's now part of #84.

Also, the custom Vim script so far is almost 2x faster than :TOhtml in benchmarks. But this isn't final,

The pushed version seems to be ~1.5-1.6x faster than :TOhtml. The times involved are now fast enough that I'm not sure how much of the difference from "almost 2x" is noise at this scale.

Maybe it could even be in lua and be faster still.

It does use Lua in Neovim where appropriate. This doesn't include code dealing with nvim_get_hl() because some versions of Neovim in our supported range are known to have quirks with that function when translating data between Vim dictionary and Lua table.

[NoahTheDuke]

This is sick, dude, thank you for taking care of all this.