fix(repeatable_move): remove deprecated and unused functions

[clason]

Also stop advertising as generic API and point to nvim-next instead.

[clason]

Now the module looks much more manageable. I'm still a bit unhappy that all its functions are exposed; I'd like to minimize the API surface here so we can only document what is needed. Suggestions are welcome, as I currently don't have a good grasp of what is actually public and what can be local.

[clason]

I'm also wondering whether we can't do away with most of the convenience wrappers; we can map the "standard" keys as part of setup and leave custom mappings to just use the base repeatable_move function?

[kiyoon]

Don't you think it will be much more difficult to maintain if you make a setup option for this?

[kiyoon]

Like, if we put it in the setup, it is way unclear that we're remapping the default important keys which can lead to more incompatibilities with other plugins.

Also, the convenient wrappers make it much more intuitive to configure and know how it works. It requires only the 8 wrappers and most of the time you won't even have to touch them.

I wouldn't worry about making stuff local because they are required in another module (move.lua). And if we didn't document them, it means we can just break them without warning. It is user's responsibility if they use an API that is private.

[clason]

Like, if we put it in the setup, it is way unclear that we're remapping the default important keys which can lead to more incompatibilities with other plugins.

Well, that would depend on how it is put in setup, which is TBD.

I wouldn't worry about making stuff local because they are required in another module (move.lua).

No, only make_repeatable_move is. make_repeatable_move_pair is not used at all, as far as I can tell; neither is clear_last_move.

And "opposite", "next", and "previous" are simple one-line wrappers; I don't much see how the wrapped function call is any less explicit.

And if we didn't document them, it means we can just break them without warning.

Yes, but how would they know which are private in which are public? Right now, they're all exposed and documented.

[kiyoon]

And "opposite", "next", and "previous" are simple one-line wrappers; I don't much see how the wrapped function call is any less explicit.

They are necessary, and I would not make users put that in their config to set their keybindings. If it's in our module, we can change the form however we want, but if it in theirs, we need to ask everyone to change the config to suit the new form of a refactor.

No, only make_repeatable_move is. make_repeatable_move_pair is not used at all, as far as I can tell; neither is clear_last_move.

If those aren't used, you can remove them.

Yes, but how would they know which are private in which are public? Right now, they're all exposed and documented.

You just removed the comments and the documentation, so I think it's users' responsibility if they looked at the source code and decided to make use of them.

Or, you can mention that the API is deprecated and should not be used anymore in comments (or README) so existing users can know this and be ready to opt out.

[kiyoon]

If the setup function adds all kinds of keymaps, I think it makes it harder to use custom plugins to hook into the keymaps (like the repeatable move plugin), and also hard to use the lazy.nvim keymaps to lazy-load the plugin. So my preference is that the plugin just expose minimal APIs to use the functionality and let users to set keymaps themselves.

[clason]

lazy.nvim keymaps to lazy-load the plugin

That is a fool's errand. We should make sure that the plugin itself is "lazy" enough that people don't need that.

So my preference is that the plugin just expose minimal APIs to use the functionality and let users to set keymaps themselves.

Mine as well, but it's been pointed out that in this case that would lead to too much boilerplate (see new README, compare to the old), so some setup for textobject mappings is heavily requested.

[kiyoon]

That is a fool's errand. We should make sure that the plugin itself is "lazy" enough that people don't need that.

Really? What if some people prefer using that? Is making the plugin "lazy" also not related to treesitter features and it requires extra maintenance burdens to be compatible with many plugin managers?

Mine as well, but it's been pointed out that in this case that would lead to too much boilerplate (see new README, compare to the old), so some setup for textobject mappings is heavily requested.

Okay, as long as it still has the old way, it should work. But I'm sad to see that even if I made a repeatable-move plugin, I have to force everyone to stop using the setup function, and thus it won't even be possible to easily persuade people to adapt to the change.

[clason]

Really? What if some people prefer using that? Is making the plugin "lazy" also not related to treesitter features and it requires extra maintenance burdens to be compatible with many plugin managers?

Then it's on them. Plugin managers should support plugins, not the other way around.

But I'm sad to see that even if I made a repeatable-move plugin, I have to force everyone to stop using the setup function, and thus it won't even be possible to easily persuade people to adapt to the change.

We do need a setup function for the textobjects and movements themselves. Whether we add the repeatable movements to that or not is still under discussion. (I would prefer it over exposing 8 (eight!) different wrappers that are small variations on a theme, but if there is a good(!) reason to keep the API separate, then that's fine.)

Anyway, I'm done with the changes; still not convinced of the value of repeat_last_move_{opposite,next,previous} but will leave them alone. I would appreciate one last, good, look that I didn't break anything.

[clason]

Well, maybe there's one last change I'd like to make: I'm not crazy about the dependence of move on repeatable_move. Maybe we can combine the two modules? What do you think?

[kiyoon]

I took a look and it looks okay to me, although, I haven't tested it.

If the move functions are mapped during setup, we can't really make them repeatable because each keybinding should register the last_move and I guess there's no good way to do it.

[clason]

How does that work on master then, where the mappings are set inside the setup?

[kiyoon]

In master, repeatable move API was already a part of the plugin, so we didn't need to worry about supporting the plugin itself. But now, because there will need a separate plugin, the plugin has to be able to register all keymaps that does movements.

I think nvim-next also had the same issue and thus implemented a hacky way to intercept the setup and map on their own using the setup table

[clason]

In master, repeatable move API was already a part of the plugin, so we didn't need to worry about supporting the plugin itself. But now, because there will need a separate plugin, the plugin has to be able to register all keymaps that does movements.

No, that's a misunderstanding: You will not require a separate plugin. I'm just refactoring (and removing support for other plugins). Our mappings should continue to be repeatable out-of-the-box; it's just for other mappings that you'll require a separate plugin.

(That's why I'm asking about moving the repeatable_move functionality into the move module.)

[kiyoon]

See #369

You will need a separate plugin to hook into nvim-treesitter-textobject because, nvim-treesitter-textobject wants to use the same key (; and,) as the other repeatable plugin. And you just stopped people to extend its functionality.

[clason]

Well, that sounds to me like we should define and implement a standard hook instead of rolling our own. Otherwise you have fifty plugins all defining their own "make repeatable" and people have to pick one and add wrappers over wrappers for the other 49. This is just unsustainable.

[kiyoon]

If you think hook makes more sense, okay, but my idea initially was that you do not implement keymapping in the setup so it's easier for people to freely do what they need to do. And it is actually less of a burden to maintain, as you said. Maybe there could be another nvim-treesitter-textobject-keymap plugin to help people set it up lol

[kiyoon]

honestly the keymap can be more easily configured with a simple helper function.. and you can put the example in the Readme to avoid the duplication, but not put it in the plugin so we don't have to maintain it. A lot of the mappings work like that (including my LSP mappings)

[kiyoon]

I just don't want to bring back the attach.lua nightmare. It was very difficult to maintain it

[clason]

If you think hook makes more sense, okay, but my idea initially was that you do not implement keymapping in the setup so it's easier for people to freely do what they need to do.

Bring this up with @ObserverOfTime, who has been very loudly demanding the setup back ;)

I just don't want to bring back the attach.lua nightmare. It was very difficult to maintain it

Oh, my, no; killing that with fire is the whole point of the 1.0 refactors...

[clason]

Anyway, let's move forward. There are three options here:

1. drop all commits but the first (removing deprecated functions) and move on with my life;
2. merge this as is for now; discuss integration with repeatable movement plugins later;
3. also merge repeatable_move into move.lua for even deeper integration and reduced code duplication/dependence.

Your choice.

[kiyoon]

I hope you understand what the problem is with the repeatable move integration by now.

So here's my proposal:

In Readme, we make it

local function map_select(key, query, capture_group)
  vim.keymap.set({ "x", "o" }, "af", function()
    require "nvim-treesitter-textobjects.select".select_textobject(query, capture_group)
  end)
end

-- To be consistent with move and default keybindings I changed this to "am".
map_select("am", "@function.outer", "textobjects")
map_select("im", "@function.inner", "textobjects")
-- because "c" is usually for comments
map_select("al", "@class.outer", "textobjects")
map_select("il", "@class.inner", "textobjects")
-- You can also use captures from other query groups like `locals.scm`
map_select("as", "@local.scope", "locals")

local function map_move(key, direction, location, queries, capture_group)
  local move_fun
  if direction == "next" then
    if location == "start" then
      move_fun = require("nvim-treesitter-textobjects.move").goto_next_start
    elseif location == "end" then
      move_fun = require("nvim-treesitter-textobjects.move").goto_next_end
    else
      move_fun = require("nvim-treesitter-textobjects.move").goto_next
    end
  else
    if location == "start" then
      move_fun = require("nvim-treesitter-textobjects.move").goto_previous_start
    elseif location == "end" then
      move_fun = require("nvim-treesitter-textobjects.move").goto_previous_end
    else
      move_fun = require("nvim-treesitter-textobjects.move").goto_previous
    end
  end

  vim.keymap.set({ "n", "x", "o" }, key, function()
    move_fun(queries, capture_group)
  end)
end

map_move("]m", "next", "start", "@function.outer", "textobjects")
map_move("]M", "next", "end", "@function.outer", "textobjects")
map_move("]]", "next", "start", "@class.outer", "textobjects")
map_move("][", "next", "end", "@class.outer", "textobjects")
map_move("[m", "previous", "start", "@function.outer", "textobjects")
map_move("[M", "previous", "end", "@function.outer", "textobjects")
map_move("[[", "previous", "start", "@class.outer", "textobjects")
map_move("[]", "previous", "end", "@class.outer", "textobjects")

-- You can use lua patterns to group multiple queries.
map_move("]o", "next", "start", "@loop.*", "textobjects")

-- You can also pass a list to group multiple queries.
-- map_move("]o", "next", "start", { "@loop.inner", "@loop.outer" }, "textobjects")

-- You can also use captures from other query groups like `locals.scm` or `folds.scm`
map_move("]s", "next", "start", "@local.scope", "locals")
map_move("]z", "next", "start", "@fold", "folds")

-- Go to either the start or the end, whichever is closer.
-- Use if you want more granular movements
-- Make it even more gradual by adding multiple queries and regex.
map_move("]d", "next", "start_or_end", "@conditional.outer", "textobjects")
map_move("[d", "previous", "start_or_end", "@conditional.outer", "textobjects")

1. drop all commits but the first (removing deprecated functions) and move on with my life;
2. merge this as is for now; discuss integration with repeatable movement plugins later;
3. also merge repeatable_move into move.lua for even deeper integration and reduced code duplication/dependence.

I don't mind 2 as long as, setup function doesn't map keys. I think it will cause other hard incompatibilities between plugins

[clason]

I hope you understand what the problem is with the repeatable move integration by now.

Yes, I do; thank you for your patience. I hope you also understand what my problem with it is by now ;)

I like your proposal (and personally prefer it over the setup as well). I would be fine with adding these wrappers in the main module, too.

One issue (that the old attach.lua was meant to solve) is that people will likely only want to map these if the textobjects actually exist. So I would propose to add a check for the corresponding query (and maybe parser?) in the wrapper and document these as buffer-local mappings in a suitable autocommand.

(I'm fine with adding new functions as long as we delete enough old functions to make room for them ;))

(And I would be interested in your thoughts on option 3.)

[kiyoon]

Well, if the function doesn't do anything when it doesn't exist, I think most people won't mind at all actually. Advanced uses can make the check function themselves as it's going to be a simple one-liner. If this becomes part of the plugin, then people will depend on the function thus nothing is different than having a setup function mapping keys.

I guess, I know how to hack it so it won't be a problem for me, but it's just that it's harder for people. You can do as you want, I think most people don't care actually.

About option 3: I think it's better to put as a separate module as it looks more readable and the dependency is more clear.

[clason]

Well, if the function doesn't do anything when it doesn't exist, I think most people won't mind at all actually.

They will if they want to override builtin mappings like ]m. It's the same problem as with LSP mappings overriding builtin mappings like K and gD.

[kiyoon]

OK, I guess we need a mapping helper then, for sure

[clason]

We can use shared.check_support for that, although that would require pulling that in for the main module, which I'd prefer not to. It may be better to set up a simplified version; I'll have to think about this. Anyway, that's a separate discussion.

[clason]

We can use shared.check_support for that, although that would require pulling that in for the main module, which I'd prefer not to.

On the other hand, if we move the support check to the time of definition rather than execution (which I absolutely think we should), then we can move check_support and friends to the config module and keep things lazy.