pasky / claude.vim

Claude vim plugin for AI pair programming - a hacker's gateway to LLMs
MIT License
216 stars 12 forks source link

Contribution guide #18

Open simlei opened 1 week ago

simlei commented 1 week ago

Hi, I have made several improvements to the plugin locally already. I would really love to contribute them back as individual patches.

Right now it's not ready for PR submission and I will have to do this during a weekend; For contributing PRs & their formatting, I would like to already ask if are there any guidelines you'd like to offer up.

What I have are some concentrated changes:

As mentioned, a number of these changes will require "public" functions for registering such hooks into the core logic of the plugin; thus a section with a set of public interface functions (as oppesed to s: script functions) would emerge.

During local development I have also strewn quite some logging hooks throughout the plugin code. They are disabled by default. They give a lot of insight into why some current behavior is not as intended and I would like to keep them in there due to the inherent non-reproducibility of chat outcome to make spontaneous fixes easier. (But disabled by default of course). I could see how one would not like to have such code in a PR so please advise.

I'm really enjoying your plug-in so much, thanks a million!! Many thanks for keeping it a vimscript plugin so that it could be enjoyed by both the vim and the nvim crowds which sadly have diverged so much in the last years...

pasky commented 4 days ago

Hi @simlei , that sounds amazing!

I don't think there are any particular guidelines for me to offer, except to if possible keep the logically separate changes (including the addition of logging hooks) as separate commits, so that they can be reviewed easier and we keep a reasonably clean history.

P.S. re capturing the Changes - I think the current solution is pretty dirty actually, and I wonder if it could be somehow more unified with how we handle tool invocation. (Actually, if we made proper use of the caching API, we could just make them tool invocations w/o extra cost, I suspect?) Had this idea for a long time but unfortunately I don't have much time to develop this plugin myself right now.

simlei commented 3 days ago

Ok, sounds good. I feel I could split up my changes into such units.

How do you feel about eventually splitting up the monolith .vim file into logically separate units? After having looked into how information flows in the plugin I have observed it is reasonably clean if I look at the main stakes i.e. what is prompted, querying and receiving assistant input, and converting that to actionable changes. However I have also some concerns re: code structure:

1) error handling: abort is generally not used for functions, meaning, any non-modeled error path will cause havoc (program will continue on error...) 2) allocation/deallocation of state:

And then there's the issue of extracting the assistant's changes as you have mentioned:

That could be easily made into a simple binary choice of how the assistant should respond with a code block, and captured via structs (dicts), making changes probably be correctly put into diff form in 99% of the cases.

My main reason of going to the issue of splitting the monolith vim file up comes from the desire to continue using the plugin to improve the plugin, which I think gets hard as the plugin exceeds 1000LOC in one file. But also from the desire to trimming out the fat from the information flow. The things with "allocation / deallocation of state" I lament would be handleable by a bit of boilerplate commands (e.g. command -nargs=1 WithCurrentBufPreserved <expression>) and combined with more structured state, there could be each <= 100LOC files for parsing, communication formatting, and most importantly, top-level use case functions (use cases "chat" + "implement") that would contain all of the logic of that define that use case, without any of the "logistics" that often one would like to not change up at all (assistant network & IPC, vim buffer handling). I think with such refactoring, this plugin would continue to be in great shape in the future.

pasky commented 3 days ago

I'm totally fine with the separation at this point. (At the beginning, I wanted to prove a point with keeping the plugin really small for the amount of power it wields, but now I think it's a good idea to clean it up and separate a bit. Though I would still like to keep the philosophy of a small codebase in general.)

What about we control the scope a bit, though?

pasky commented 3 days ago

One argument for a single file .vim is making sure Claude has the whole "codebase" as a context when working on this. The ability to use Claude to code claude.vim would be one thing I'd be concerned about. Can we figure out how to preserve it (without having to have 10 windows open :) maybe it's just about not going over 2-3 files for now, then having to have all of them open is ok).

(Ultimately, we'll want to use a smarter way to build context, I think there are several existing OSS tools we can leverage by now...)