feat: handle leading whitespace

[benlubas]

closes #85

Adds an off by default configuration value, handle_leading_whitespace. When true, otter will track the leading whitespace of each capture, and when a command is called, properly adjust values where it needs to.

screen_cap.mp4

The formatting bug is present in main, it appears to behave the same way in this branch as it does in main, I opened #87 to document that.

[benlubas]

Oh wait, I didn't test rename. One sec

[benlubas]

Officially ready for review.

[jmbuhr]

I fixed #87 in #88. This PR also added more examples based on usecases I am currently aware off, which now also includes a norg example. Can you merge main into this branch and play around with the examples? ./tests/debug-example <some ft or number> opens the respective example with the otter.dev_setup.
The markdown and quarto examples should just work with your setup. The ts example just needs the typescript treesitter grammar (in case you don't have that). The rust example uses some custom injections file, so don't worry too much about that for now.

[jmbuhr]

Also, feel free to add to the examples if you encounter edgecases.
Since a lot of things depend on the interaction of plugins and the existence of various language servers plus TS grammars, I haven't found a nice way of testing end-to-end automatically, yet, hence all the examples.

[benlubas]

This happens when formatting indented blocks. I can fix that tomorrow. Otherwise, this looks good to me.

ahh, I messed up the commit history I mean, what no, the history was fine this whole time :)

[benlubas]

@jmbuhr do you know of another language server that does range formatting? Lua LS seems to do something that doesn't make sense and it results in that last line shifting.

Would be good to have another LS to test with.

I've tried pyright and rust analyzer.

[jmbuhr]

R's language server does formatting, and it also currently works in e.g. quarto documents. So that's a good reference.
(See https://github.com/quarto-dev/quarto-nvim?tab=readme-ov-file#r-diagnostics-configuration to avoid annoying whitespace diagnistics because of the otter document)

[jmbuhr]

Currently on main, lua only adds one line when formatting the last chunk, so maybe it just wants a blank line after the last piece of code.

[jmbuhr]

oh, wait, there is more off-by-one stuff going on. After my fix for #87, formatting an R chunk now refuses to format the last line of a chunk. i.e.

stays, but

becomes

[benlubas]

Yeah this is very weird. I have something that works for lua ls right now. But I will test with R after class or when I have time.

screen_cap.mp4

[benlubas]

I think I have an idea of whats happening. I think Lua LS is ignoring the column info and only using the line numbers.

And R LS is using the col numbers correctly.

Lua LS then, when sent 21, 0 | 22, 0 will try to format two lines in the other buffer. One will be print the other a blank line with nothing below it. And it'll respond with "format print and delete the extra blank line below it" which was removing the @ end before.

Subtracting one from line number fixes that for Lua. But now r LS is getting 21, 0 | 21,0 and it's like "you want me to format nothing? Okay here you go"

If this is correct I think a fix would be to subtract one from the end line number and correctly set the end col number to the length of the line.

[jmbuhr]

ah, indeed, I was wondering about this halfway during implementing the column-number fix and then forgot about it when it just worked for the examples I tried. Do you happen to know if there is a shortcut for "to the end of the line" instead of having to add the count? e.g. some nodes report their range formatted as
[<line>:<col> - <col in the same line>] in vim.treesitter.inspect_tree but I have not seen any range that omits the column.

[benlubas]

I'm not familiar with a shorthand for it

[jmbuhr]

Small heads up: My attempts at fixing broke too many things, so I reset main and moved those fixes back to their own branch as I should have done earlier (#89)

[benlubas]

@jmbuhr I'm unable to get the R LS to range format. Is there an additional package that you have to install?

I restarted nvim a few times and it started behaving.

[benlubas]

Uhhh. why does it refuse to format y = to y <-?

I've checked, the response from the LSP is actually telling us to keep y = it's not a problem with my implementation.

In other news. The different ways that R and Lua LSPs format is a good way to make sure we're doing things correctly. I wish I could spend more time on this to get it working, but it will probably have to wait another day.

[benlubas]

@jmbuhr I have an implementation that works for pyright, R's LS, and lua_ls. But the implementation does get a little messy. There are a few different ways for the LSP to respond with a "change this text" request, and we have to account for all of them. There might be more that I'm missing I'm not sure.

There has to be a better way to do this.

One approach that would definitely be neater (although less performant) would be to just recursively search the table for range keys, b/c the spec is at least nice in that all ranges are the same, and they're all called range. But there are a lot of fields that could contain a range, and I'm currently handling all of them. This is kinda a lot (and a lot of potential maintenance overhead), and I'm not even sure that I've covered all of them.

I'm thinking that it would be nice to have access to this position data after neovim's lsp code has processed it for us (hopefully into a more uniform set of change instructions). But I don't know if that's possible, and suspect it isn't.

[jmbuhr]

We currently have two ways of changing what the lsp response does. The first is by using the filter function to modify the response itself. The second is by swapping out the handler that processes the response (after potentially being filtered). This became necessary when the nvim default handler for hovers stopped showing hovers from other buffers. Maybe the code for the builtin handler offers some clues, or can be taken and partially replaced.

[jmbuhr]

Tough your solution already looks good, given the complexity of the task.

[benlubas]

Perhaps my logic should go into a filter function then? Although, these changes apply to every request instead of only certain ones so it might make more sense to leave it where it is.

I have read through the lsp help page, but I couldn't find anything that would help us here.

[benlubas]

The handlers have the same information available to them that we have, and no additional information as far as I could tell. So I think swapping them would work but it would look pretty similar to what we're already doing.

[benlubas]

@jmbuhr how are you feeling about this PR?

[jmbuhr]

If it is good to go from your side I will test it a bit in my own workflows and then merge it :)

[benlubas]

Yeah, I've not found any other problems. I think it's good to go

[jmbuhr]

Apologies for making you merge main so often, I just merged a change to the way we iterate over the treesitter nodes (#100) so we can make use of a fix in nvim (neovim/neovim@bd5008d), which now handles offsets properly. Might even have fixed other issues along the way.

[benlubas]

Ooh this is a nasty merge. I think b/c this branch also fixes some of the offset issues that were a part of #89

Does #100 also fix all the offset issues? b/c then I could probably just accept all the new changes and wipe my stuff out.

@jmbuhr

[jmbuhr]

It doesn't address the issues in https://github.com/jmbuhr/otter.nvim/pull/89/files directly, though the upstream changes in nvim that this pulled in may have fixed some issues as a side effect. The most apparent thing fixed is where an injection would include e.g. wrapping quotes in the otter buffer, before this proper handling of offsets.

The only change in #100 is actually just replacing 3 occurences of

for id, node, metadata in treesitter_iterator.iter_captures(root, main_nr, query) do

with

  for pattern, match, metadata in query:iter_matches(root, main_nr, 0, -1, { all = true }) do
    for id, nodes in pairs(match) do
      local name = query.captures[id]

      -- TODO: maybe can be removed with nvim v0.10
      if type(nodes) ~= "table" then
        nodes = { nodes }
      end

      for _, node in ipairs(nodes) do

and adding the two more ends after the loop body accordingly.
It just looks more complicated in the diff because github apparently thinks aligning one end is worth shifting everything else for.

So it fixes the issues with the "offset" directives, but probably not the issues we found where we had an offset in line numbers etc.

[benlubas]

ah that helps a lot, ty

[jmbuhr]

I think you can take main and add the lines where you use text, leading_offset = trim_leading_witespace(text, main_nr, row1) pretty much in their old positions.

[benlubas]

I forgot that I was testing with only python enabled just to make sure that setting a language list worked, and I spent like 10 minutes trying to figure out why it couldn't find the lua code cells in my document 🤦‍♂️

I think this is ready now.

[jmbuhr]

EDIT: forget what I just wrote, I had your feature disabled...

[jmbuhr]

I'll merge and if I don't experience any issues, I will turn it on by default, haven't found any downsides so far.

[jmbuhr]

@benlubas I did some profiling with https://github.com/stevearc/profile.nvim and this doesn't even introduce any performance hits. Great job on the implementation!