nvim-treesitter / nvim-treesitter-textobjects

Apache License 2.0
2.2k stars 199 forks source link

refactor(repeatable_move): extract the `MoveFunction` interface #671

Closed ofseed closed 3 months ago

ofseed commented 3 months ago

repeatable_move is out of the plugin's scope, and I agree with simplifying it. But even if the make_repeatable_move_pair was removed, the current API is good enough for users to define their repeatable move, it only needs some refactoring and documentation. Also contains some tiny improvements for type annotations.

nvim-next does not support the main branch, so there's no replacement for users now, but since its author knows this so it might be implemented in the future, so I kept the reference to nvim-next. see https://github.com/ghostbuster91/nvim-next/issues/22.

Edit: I changed this PR to feat because the MoveFunction interface should be guaranteed.

kiyoon commented 3 months ago

This is intended to be dropped out from the README to limit the support and make it easier to maintain. Now it's not intended to be used with other plugins.

ofseed commented 3 months ago

Currently, no plugin can replace this functionality. nvim-next is based on the previous module system, and more importantly, I think the current implementation is already simple enough to be easily maintained.

The purpose of this PR is merely to show users how to use an already exposed API.

I think the better approach would be either not to provide this functionality at all (which would be the easiest to maintain) or to expose the internally used method in the most maintainable way (which is the current situation). After all, we have already implemented this feature. It seems odd to keep a feature that we originally intended users to implement with other plugins.

Or do you mean that guaranteeing the make_repeatable_pair API would also harm maintenance?

kiyoon commented 3 months ago

I honestly agree with you that is has been stable and simple enough to maintain. However, there was another voice that any dependency with different plugins should be completely removed so we can focus more on the treesitter features. See the discussion at #666.

I think I would personally implement my own version of repeatable movement plugin (which is basically a copy of the original implementation) once I transit to the main branch.

ofseed commented 3 months ago

Except for adding an explanation on README, this PR also did some refactoring, which made the internal API more flexible and avoided some diagnostic errors. Could still consider merging it even if additions to the README are not needed (I can or you can delete it).

kiyoon commented 3 months ago

Any reasoning as to why query_strings and query_group are removed from MoveOpts and instead placed as the move() function parameters?

ofseed commented 3 months ago

Any reasoning as to why query_strings and query_group are removed from MoveOpts and instead placed as the move() function parameters?

To keep the repetable_move module independent. The original motive is to make it easier to expose API, but I think it should also be reasonable even if we do not expose it.

ofseed commented 3 months ago

If we decide to remove the repeatable_move module one day, we can only remove the opts parameter. It should be a good refactor from both perspectives.

kiyoon commented 3 months ago

I think we won't remove the repeatable_move itself, but we're not exposing the API. So, users may use it with this plugin but they're not supposed to use it with other plugins. Of course, they can hack it, but that won't be guaranteed to be stable and is out of scope for maintenance.

Given that in mind, you can remove the README and further refactor if you wish. I think what you did is great and I appreciate it.

ofseed commented 3 months ago

Okay, I deleted explanations on README and squashed these commits into three to avoid being overly finegrained.