feat: add option for print_var for normal mode attempt to find var under cursor

[teddylear]

feat: Split print_var and print_var_norm behaviors so we can iterate on print_var_norm

As discussed in #321 and from other issues/PRs raised, we can have a normal mode print_var function (in this case print_var_norm) to get variable under cursor in normal mode while leaving the old print_var that uses visual mode in place.

@pranavrao145 Let me know if you want go this route (I'm alright to scrap this if we want to do something else). I figured this way we have the normal print var people have been asking for and can iterate on that, while still having the visual mode option. For not it's just a copy paste with one func difference, we can clean up the internals later but have these difference exposed interfaces. This way we have a fallback but give users what they want as well.

Also please double check my doc changes. I think I updated the required places, but I want to make this clear to end users.

[pranavrao145]

@teddylear since the current behaviour uses normal mode by default if nothing is selected, then optionally visual mode as a fallback, I'm not quite sure what splitting up commands for the two modes will buy us. As of right now, to access both behaviours, the config can be set up like this:

vim.keymap.set(
  "n", -- normal mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({})<CR>",
  { noremap = true }
)
vim.keymap.set(
  "v", -- visual mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({})<CR>",
  { noremap = true }
)

With splitting them up, it will be pretty much the same thing but with print_var_norm for the normal mode remap instead of just print_var, which is why I'm not sure what it will do for us.

However, after reading #321, I am realizing that the docs for print_var state that the remap must be made in visual mode, which is misleading and causing users to think this is the case. If we update the docs to state that either can be used, will that not solve the issue? Later on, if we want to upgrade the command's capabilities in normal mode (to find class fields and such using treesitter), I think we can even just update the code in the get_variable function later on under that large if statement. Let me know what you think about this.

Also, if we were still to split these, perhaps we could do it with a { normal = true } in the opts? Will likely mean less code migration overall, as well as one less function to keep track of. The resulting map would look extremely similar, something like:

vim.keymap.set(
  "n", -- normal mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({ normal = true })<CR>",
  { noremap = true }
)

Apologies for the long comment - I'm not sure if everything I said quite made sense, let me know if you have any questions and I can clarify.

[pranavrao145]

To follow up, I've opened #323 to show what a docs update would look like to clear up the discrepancy for the end user. Take a look and let me know what you think!

[teddylear]

@pranavrao145 Thanks for taking a look at this. Agreed this isn't optimal, let me edit this PR to go the config route where we default visual, but can optionally attempt to find var under cursor in normal mode. I'll comment again once ready for re-review. I'll take a peek at your doc update as well.

[teddylear]

@pranavrao145 Updated PR with implementing it another way. Lmk if you would like me to do it another way you mentioned, here I just set it as a config on the plugin itself so users can globally turn it on and off depending if they want to opt into it.

[pranavrao145]

@teddylear so I'm thinking about this a bit more, and there are two issues that I'm seeing arise:

1. Putting the option in the global config (setup function) will essentially force users to always choose between one or the other. I was more thinking that the user could pass in the opts in their mapping itself, like we do in printf with below = true or below = false.

Example map would be (with the normal = true):

vim.keymap.set(
  "n", -- normal mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({ normal = true })<CR>",
  { noremap = true }
)

That way, the user can make two separate maps like this, still using both modes whenever they want instead of choosing one for always:

vim.keymap.set(
  "n", -- normal mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({ normal = true })<CR>",
  { noremap = true }
)

vim.keymap.set(
  "v", -- visual mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var({ normal = false })<CR>",
  { noremap = true }
)

I think doing it this way, passing in the opts for each function (like in printf) gives the user maximum control.

2. The only thing about problem number 1 is that doing it is basically pointless because the plugin actually already supports this behaviour. The docs were outdated, but that's fixed by docs(print-var-normal-docs): updated docs for inline var and print var to reflect proper modes #323. Essentially, right now, if the user makes these maps (this is the same set of maps from before without the normal = true):

vim.keymap.set(
  "n", -- normal mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var()<CR>",
  { noremap = true }
)

vim.keymap.set(
  "v", -- visual mode map
  "<leader>rv",
  ":lua require('refactoring').debug.print_var()<CR>",
  { noremap = true }
)

This works, and the plugin already is able to pick up which mode we're in and do whatever the user wants. If they're in normal mode, it automatically finds the var, and if it's visual, it automatically prints the selection. With the current behavior of the plugin, the user can already use whichever mode they want by default, and always switch to the other one if they're unhappy (even using the same map). So what I'm wondering is if we even need to make any changes to this at all. If we put in the opts, I think it would just be a matter of adding in { normal = true } to the current config (like the maps in 1.), which might be a little redundant.

There was some confusion in the commit history due to reverts and whatnot, as well as a lag in docs (my fault), so I think we may have confused ourselves and the end user (in that, the behaviour #321 asks for already existed when it was opened). Anyways, take a look and let me know how you want to move forward on this.

[teddylear]

@pranavrao145 I still think we need the change you mentioned in your last message because of this piece of code here. This means if we go from visual mode once and highlight something, then when going back to normal mode the print_var operation for normal mode is broken because the region is not all zero values. This means that what was highlighted will be printed, not what is under the cursor. I do like the way you implemented it, because then you have two separate keymaps for using normal mode vs visual mode. I'll update my PR and post when ready for re-review.

[teddylear]

@pranavrao145 Ok updated PR, please let me know what you think. For testing I'm using the following key binds

     map(
          "v",
          "<Leader>dv",
          ":lua require('refactoring').debug.print_var()<CR>",
          {
               noremap = true,
               silent = true,
               expr = false,
               desc = "Refactoring plugin debug print var",
          }
        )
  
     map(
           "n",
            "<Leader>dn",
            ":lua require('refactoring').debug.print_var({ normal = true })<CR>",
           {
              noremap = true,
              silent = true,
              expr = false,
              desc = "Refactoring plugin debug print var",
           }
     )

[pranavrao145]

@pranavrao145 I still think we need the change you mentioned in your last message because of this piece of code here. This means if we go from visual mode once and highlight something, then when going back to normal mode the print_var operation for normal mode is broken because the region is not all zero values.

Ohh okay fair enough, I failed to understand the problem - sorry about that. Thanks for the explanation and the PR changes, let me take a look again and I'll let you know.

[pranavrao145]

LGTM!