CKolkey / ts-node-action

Neovim Plugin for running functions on nodes.
359 stars 20 forks source link

`toggle_multiline` ignores whitespaces #29

Closed matthias-Q closed 1 year ago

matthias-Q commented 1 year ago

I want to use toggle_multiline() in SQL therefore I have specified the two node types in my config like this:

sql = {
    ["select_expression"] = require("ts-node-action.actions").toggle_multiline(),
    ["column_definitions"] = require("ts-node-action.actions").toggle_multiline(),
},

Here is a SQL snippet and Treesitter AST as an example:

create table tab (
    a int,
    b float,
    c text
);

AST:

statement [0, 0] - [4, 2]
  create_table [0, 0] - [4, 1]
    keyword_create [0, 0] - [0, 6]
    keyword_table [0, 7] - [0, 12]
    table_reference [0, 13] - [0, 16]
      name: identifier [0, 13] - [0, 16]
    column_definitions [0, 17] - [4, 1]
      column_definition [1, 4] - [1, 9]
        name: identifier [1, 4] - [1, 5]
        type: int [1, 6] - [1, 9]
          keyword_int [1, 6] - [1, 9]
      column_definition [2, 4] - [2, 11]
        name: identifier [2, 4] - [2, 5]
        type: float [2, 6] - [2, 11]
          keyword_float [2, 6] - [2, 11]
      column_definition [3, 4] - [3, 10]
        name: identifier [3, 4] - [3, 5]
        type: keyword_text [3, 6] - [3, 10]

When I trigger toggle_multiline() to collapse the column_definitions the whitespaces between the identifier and the type are lost

result:

create table tab (aint,bfloat,ctext);

How can I fix this?

I am using the SQL grammer that is shipped with nvim-treesitter.

alexpw commented 1 year ago

On it's own, toggle_multiline() doesn't know how to format the spacing between collapsed nodes, so it accepts the argument padding (a table) to help it.

In the column definition example, to have a space precede the type and a space to follow after each , the padding table would be:

local padding = {
  [","]     = "%s ",
  ["int"]   = " %s",
  ["float"] = " %s",
  ["text"]  = " %s",
}
-- ...
sql = {
    ["select_expression"] = require("ts-node-action.actions").toggle_multiline(padding),
    ["column_definitions"] = require("ts-node-action.actions").toggle_multiline(padding),
},

and, for your example, will collapse to:

create table tab (a int, b float, c text);

You'll obviously need to fill out the rest of the relevant sql grammar, for types in this case, but that'll get you started.

matthias-Q commented 1 year ago

Ah ok, thanks. I was hoping that I just need to specify the separator and it would just keep all of the child nodes intact.

CKolkey commented 1 year ago

@alexpw is spot-on - Because tree-sitter has no knowledge of the spaces between nodes, we have to help it out a bit by providing that. Thus far I haven't found that to be too troublesome, since languages have a limited number of tokens, even fewer of which are "unnamed".

If you work out a nice SQL implementation, I'd welcome the contribution :)

matthias-Q commented 1 year ago

Yeah, SQL is somewhat special since multiple tokens can follow each other and then separated by a ,.

I will see if I can dig into it.

alexpw commented 1 year ago

Yeah, SQL is somewhat special since multiple tokens can follow each other and then separated by a ,.

Python is like that too, eg, is not, not in, so take a look at the padding in filetypes/python.lua for an example of how to handle that.

matthias-Q commented 1 year ago

Ah, now that I play around with it I realized, that the padding table does contain tokens and not the node names. Wouldn't it be better to use TS nodes instead?

If I use tokens, I have to include everything in uppercase and lowercase (SQL does not care if it is int or INT)

CKolkey commented 1 year ago

To my knowledge, there's not an API to define unbound TSNodes, and given that these are already unnamed nodes, the best I could come up with was to get the text. If there's room for improvement then I'm certainly all ears :)

I do get that it's a bit redundant for SQL, since it's case insensitive. I guess technically you could use string.lower() before passing it into the table, and then match the old text to see if it's changed and string.upper() after if needed... but that seems like more work than just having two entries for each text 🤷🏼

matthias-Q commented 1 year ago

Why not adding the padding just to named nodes for example like this (not perfect, but you do not have to specify a padding for each node type):

local function collapse_child_nodes(padding)
  padding = padding or {}

  local function collapse(node)
    local replacement = {}
    local child_text
    local context

    for child, _ in node:iter_children() do
      if child:named_child_count() > 0 then
        child_text = collapse(child)
        if child_text == nil then
          return
        end
        table.insert(replacement, child_text)
        context = nil
      elseif child:type() == "comment" then
        return
      elseif child:named() then
        context = helpers.padded_node_text(child, padding, context)
        table.insert(replacement, context .. ' ')
      else
        context = helpers.padded_node_text(child, padding, context)
        table.insert(replacement, context)
      end
    end

    return table.concat(vim.tbl_flatten(replacement))
  end

  return collapse
end

This is from lua/ts-node-action/actions/toggle_multiline.lua

Sorry if this is a naive solution. I was just digging around. It is not perfect, because there is still a trailing white space.

CKolkey commented 1 year ago

If you think that implementation makes more sense for SQL, there's no rule saying all languages need to use the "builtin" version. Would the idea for the helper be to take the node:type() and pad it accordingly?

matthias-Q commented 1 year ago

Yeah, that sounds like a good idea. However, I have not found away to deal with the last named node. This one should not be padded.

So btw this affects not only SQL. Julia would be another example. Arrays in Julia are defined like:

a = [1 2 3; 1 2 3]

So, cols separated by white spaces and and cols by ;

CKolkey commented 1 year ago

I just merged some updates to that helper - check it out. It lets you do a look-ahead type thing with a next_nil key in the padding table. That would be your last named node.

Also, I haven't really read this one thought, but there's a pretty bangin' julia implementation here: https://github.com/Oliver-Leete/Configs/blob/fd550f4bb39aa69cea8eb9137a29ff0229a0b7da/nvim/lua/ts-node-action/filetypes/julia.lua#L33

matthias-Q commented 1 year ago

@CKolkey , I was digging through the dotfiles for julia that you have posted an most of them are not working. I was able to get some of them working for julia. However, the vanilla toggle_operator does not seem to work for julia.

local operators_jl = {
        ["!="] = "==",
        ["=="] = "!=",
        [">"] = "<",
        ["<"] = ">",
        [">="] = "<=",
        ["<="] = ">=",
}
julia = {
    ["operator"] = actions.toggle_operator(operators_jl)
}

When I run the node action, the operator just disappears.

CKolkey commented 1 year ago

Hmm. Thats too bad. I hadn't tried it out myself, never used Julia. If you've got something working feel free to open a PR, and I can help sort out the operator issue :)

matthias-Q commented 1 year ago

So, the main reason of this issue has been fixed by introducing the uncollapsible table. I will close this issue now.

Should I make a PR to add these filetype specific settings for R, Julia and SQL that I have created? Some of them are still not working as a expected (one PR for which might fix that is pending)

CKolkey commented 1 year ago

Yea, feel free to open a PR - preferably one per language, just to keep it organised ;)