-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: handle leading whitespace #86
Conversation
Oh wait, I didn't test rename. One sec |
Officially ready for review. |
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? |
Also, feel free to add to the examples if you encounter edgecases. |
2bb82fc
to
3654447
Compare
3654447
to
a4f706a
Compare
@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. |
R's language server does formatting, and it also currently works in e.g. quarto documents. So that's a good reference. |
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. |
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 |
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 Subtracting one from line number fixes that for Lua. But now r LS is getting 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. |
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 |
I'm not familiar with a shorthand for it |
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) |
@jmbuhr I restarted nvim a few times and it started behaving. |
Uhhh. why does it refuse to format I've checked, the response from the LSP is actually telling us to keep 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. |
@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 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. |
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. |
Tough your solution already looks good, given the complexity of the task. |
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. |
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. |
@jmbuhr how are you feeling about this PR? |
If it is good to go from your side I will test it a bit in my own workflows and then merge it :) |
Yeah, I've not found any other problems. I think it's good to go |
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. |
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 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. |
ah that helps a lot, ty |
I think you can take main and add the lines where you use |
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. |
EDIT: forget what I just wrote, I had your feature disabled... |
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. |
@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! |
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.