C-Loftus / talon-ai-tools

Query LLMs and AI tools with voice commands
http://colton.place/talon-ai-tools/
MIT License
46 stars 17 forks source link

Model pass has different requirements from other prompts #145

Closed jaresty closed 1 month ago

jaresty commented 1 month ago
C-Loftus commented 1 month ago

I think this is more explicit and probably a good choice. I think there is a temptation to try and cram everything into one talent command and I agree that re-factoring it out probably makes it easier for us to both maintain and users to reason about

I think we can remove the anchor at the start of the command like we talked about in the other thread but otherwise looks good to me

jaresty commented 1 month ago

Thanks! Yeah I'll remove that. Do you think we should also allow destination with no source? I think it's useless to ape both to be missing, but if either exists it could be valid.

jaresty commented 1 month ago

I'll merge this for now-we can always revisit.

C-Loftus commented 1 month ago

Shouldn't we just mimic the same behavior in the grammar? Namely if source is not specified use the selected text. And if destination is not specified, paste in place? I think we can reuse the same helpers. I don't think we need two talon commands for this, unless I am misunderstanding

jaresty commented 1 month ago

The reason for the two new commands is that I don't think there's a way in the talon grammar to specify an xor in a single statement. With the existing command, you can say "model pass" and it will try to paste the current selection where it is, which is weird. You want to require or the other, I think.

jaresty commented 1 month ago

If I'm wrong about that, let me know! Another option could be to define two rules where one requires both source and destination and the other requires one or the other. Didn't try that, though.

C-Loftus commented 1 month ago

I personally feel that combining them all into one command is probably fine. It makes the .talon file cleaner and I think we want to be very clear the pass works with any combination of source and destination including if they are optional. I agree that it isn't needed to have an empty pass where it pastes the selection to the cursor but I think it follows the grammar and doesn't hurt anything

jaresty commented 1 month ago

It does cause problems because I would pause thinking about the grammar and it would often pick up "model pass" before I specified anything, which was really unpleasant.

jaresty commented 1 month ago

I have a solution that works in a single role:

"Model pass (source | destination | source destination)"

jaresty commented 1 month ago

I can submit a pr for that later if you like! Or feel free to make that change if you get to it first.

jaresty commented 1 month ago

https://github.com/C-Loftus/talon-ai-tools/pull/146