nvim-treesitter / nvim-treesitter-textobjects

Apache License 2.0
2.14k stars 191 forks source link

refactor(shared): move out Range class, memoize, code shuffle and type fixes #667

Closed clason closed 1 month ago

clason commented 1 month ago

Includes smaller code reshuffles and todos.

clason commented 1 month ago

@ribru17 sorry, this again conflicts (cosmetically) with your PR. Mostly it's just shuffling things around, though, so your logic changes should still apply pretty cleanly.

clason commented 1 month ago

Cursory tests seem to work fine, but I'd appreciate someone who actually uses this plugin to check for regressions.

clason commented 1 month ago

(The idea behind the refactors here and mentioned in the todos is to structure the modules so people will only have to pay for what they use, when they use it. It will also make it easier to work on each module without worrying about breaking other modules.)

clason commented 1 month ago

Scope textobject already fails on main; looks like the range class (or its call sites) needs a lot more nil guards.

kiyoon commented 1 month ago

@clason maybe the "lua match" matching of query names for the move feature should be removed as well. Maybe not many people use it and it just confuse them? Or make it as an option so that it is more clear that we opt-in.

But I would like to keep the "gradual movement" feature where you can set multiple query names. It is sometimes useful.

clason commented 1 month ago

Yes, I agree, globs are overkill (but lists are useful). Not sure I want to remove them in this PR, though. If we do, I would like to standardize on the signature, though (always take tables of strings) -- polyvalence is just painful.

clason commented 1 month ago

And thoughts on further splitting up shared?

(Alternative: the motion mappings themselves check and return false if not supported; the wrapper can then check the return value and call the original mapping instead: move(...) or ]m.)

clason commented 1 month ago

OK, I'm done with my murder refactoring spree for now. The next step is to rework the config (less metatable magic, more explicit) and the helpers for (buffer local) mapping setup, but that requires thought.

But before that, we should fix the many, many, regressions from master... (including getting the tests to pass again).

Would appreciate a thorough look, then I'll merge this and the CI PR as it's a strict improvement on current main.

clason commented 1 month ago

Tests pass now; will merge if nobody complains @kiyoon @ribru17

ribru17 commented 1 month ago

Thanks a lot for this, can review now

ribru17 commented 1 month ago
  • shared.check_support and shared.available_textobjects can be moved to config to more cheaply check when setting up keymaps (this would require the mapped operations to silently fail on unsupported textobjects, which I think is the better UX anyway and is how every other textobject behaves).

(Alternative: the motion mappings themselves check and return false if not supported; the wrapper can then check the return value and call the original mapping instead: move(...) or ]m.)

Agree with all this

@ribru17 sorry, this again conflicts (cosmetically) with your PR. Mostly it's just shuffling things around, though, so your logic changes should still apply pretty cleanly.

No worries, I have accepted that that PR will come with lots of rebasing haha

clason commented 1 month ago

all yours, @ribru17 !

ribru17 commented 1 month ago

lord have mercy on my soul...