renzmann / treesit-auto

Automatic installation, usage, and fallback for tree-sitter major modes in Emacs 29
GNU General Public License v3.0
369 stars 28 forks source link

Allow ts-mode install and handle requires #31

Closed wkirschbaum closed 1 year ago

wkirschbaum commented 1 year ago

This is playing around with the following: 1) allow more than one grammar to be installed ( in case of elixir and heex ) 2) allow ts-modes to prompt for install if there are no equivalent non-ts modes

renzmann commented 1 year ago

I'm a big fan of opening the PR early and often. I have one suggestion before diving in though - instead of using WIP for a title, mind converting to a draft and marking it "ready for review" once it's in a good state? I can do some color commentary in the meantime, but don't want to be too intrusive before things have settled

wkirschbaum commented 1 year ago

@renzmann of course. sorry, was in a hurry yesterday and just wanted to get some early feedback before spending too much time on it. have not used github's draft before, but will check it out.

renzmann commented 1 year ago

My initial overall impression is that the PR seems to be doing more than one thing:

  1. Introducing the new :requires keyword and functionality around it
  2. Internal structural changes to how treesit-auto tracks & caches data. These are the things like breaking apart the treesit-auto--create-alists monolith, using lookup functions instead of alists, and the toggling on/off functions for the minor mode

I'm definitely interested in both of these angles, and always willing to clean up code where we can, but it might help my review cycles if we can break them apart, unless there's a fundamental reason they need to be tied together, that I've not yet spotted. One PR would be for the new :requires keyword, along with a motivating example, and then another PR that introduces the other capabilities and/or functional refactor.

wkirschbaum commented 1 year ago

@renzmann happy to break them up, just wanted to get to the point where it actually works. this change can be put into 3 PR's imo: 1) introduce :requires for installation of mutliple grammars 2) prompt installation when only ts-modes are available 3) clean out minor mode artifacts.

Tbh, i only wanted to get it to work with elixir-ts-mode 😆... and it spiraled a bit.

Will break it up in the next few days and we can decide what should go in, or what is better to leave unchanged.

wkirschbaum commented 1 year ago

i only used cl-loop a bit more liberally, because it is pretty common in the treesit code, not necessarily in the rest of emacs.

wkirschbaum commented 1 year ago

@renzmann i don't really understand the point of the fallback, so removed it in this branch. I don't understand how it should be used. If the point of the package is to automatically remap x-mode to x-ts-mode then is the fallback not the x-mode?

This change fixes some pollution of major-mode-remap-alist and also fixes the issue where x-ts-modes without x-modes also triggers the grammar install.

I don't know how to break it up into smaller parts, because the change is a bit more complex. The requires change which went out last week does not solve the {elixir,heex}-ts- problem, so feel this is still relevant.

I probably won't have time to look at this again and won't be offended if the PR is rejected because it is a bit of a large change.

renzmann commented 1 year ago

@wkirschbaum We can remove the fallback - the history to it looked kind of like this:

  1. Original package assumed bijection between ts-mode and original-mode
  2. Created fallback alist as that bijection
  3. Turns out many different original-modes might go to the same ts-mode
  4. Deprecate fallback alist in v0.5 in favor of recipe system

Right now it has a vestigial status in case people had already customized it through the custom interface or their init.el.

Don't worry about the size of the PR, it will just take a little longer for me to review and test it, but would like some way of confirming that the elixir integration is working correctly. That may require that I make edits here and there, if that's alright

wkirschbaum commented 1 year ago

@renzmann i don't mind at all. my priority is to get elixir-ts-mode into emacs core and think by then I will rely on whichever decision was made on handling ts-modes. i tested the {elixir,heex}-ts-mode quite extensively with my branch ( and have it loaded for other modes), but don't really know which other cases I am missing.

wkirschbaum commented 1 year ago

@renzmann i believe i fixed all the warnings, but want to take a day or two to ensure I have tested the change properly.