not-poma / lazyshell

GPT powered Zsh completion script
MIT License
370 stars 26 forks source link

Explanation Mode #2

Closed Psyf closed 1 year ago

Psyf commented 1 year ago

I could refactor it a bit better for less repetitive code, but my preliminary trial fucked up the nice "spinner + replace UX" of lazyshell_completion :(

not-poma commented 1 year ago

Since the explanations can be long and multiline, I think a better UX would be to show the explanation below the prompt (where "loading..." text is) rather than adding it to the buffer. It can disappear when any key is pressed.

To avoid code duplication, it's a good idea to extract API request code into a separate function.

I think with this intro, you can pass the command as API request directly without "What is this doing?", tested it on playground.

not-poma commented 1 year ago

I think we can also use bat to produce colored markdown output if the command is present.

echo $response | bat --decorations never --color=always --language markdown

Psyf commented 1 year ago

I think a better UX would be to show the explanation below the prompt (where "loading..." text is) rather than adding it to the buffer. It can disappear when any key is pressed.

I wish my scripting skills were that good. I'll look into whether I can achieve that.

To avoid code duplication, it's a good idea to extract API request code into a separate function.

Agreed. It's straightforward if I abandon the spinner. But I love the spinner :(

Lmk / feel free to take the code so far if you find a way!

not-poma commented 1 year ago

if I abandon the spinner

Why do you need to abandon it? You can put everything from spinner init until generated_text return into a function. It can accept the text next to spinner as an argument.

wish my scripting skills were that good.

I also had zero zsh scripting skills before, so I wrote this with help of ChatGPT and Google :) good opportunity to learn new stuff

not-poma commented 1 year ago

btw if you feel you don't have time/desire to finish this I can take over and implement the rest

Psyf commented 1 year ago

I'll take another crack in a couple hours. If I don't commit in 6 hours, feel free!

On Sat, 4 Mar 2023, 5:48 pm not-poma, @.***> wrote:

btw if you feel you don't have time/desire to finish this I can take over and implement the rest

— Reply to this email directly, view it on GitHub https://github.com/not-poma/lazyshell/pull/2#issuecomment-1454681688, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADIRKP3IHGLZEIA4PFAJB3LW2MFXLANCNFSM6AAAAAAVPKPUEA . You are receiving this because you authored the thread.Message ID: @.***>

Psyf commented 1 year ago
  1. abstracted the llm call out.
  2. moved the explanation to a newline,
  3. and adjusted the cursor so that you can edit the command without moving back all the way from end of explanation
Psyf commented 1 year ago

Couldn't figure out how to clear out the buffer to only $buffer_context on any keypress (like zsh-autosuggestions). If you want that, you can merge this and make another commit when you figure that out 😄

not-poma commented 1 year ago

This looks really good! I was thinking about displaying the explanation with something like zle -R and then waiting for a keypress, rather than appending it to the buffer.

Also maybe move all error handling into __llm_api_call and return either the correct api answer or print the error and return (move a bit more duplicated code into the function)

not-poma commented 1 year ago

btw what ZSH_AUTOSUGGEST_CLEAR_WIDGETS does?

Psyf commented 1 year ago

btw what ZSH_AUTOSUGGEST_CLEAR_WIDGETS does?

It removes the suggestions by zsh-autosuggestion (very popular plugin in oh-my-zsh from what I gather). If I don't do this, the suggestions fuck up the buffer / view.

Psyf commented 1 year ago

After a lot of experimentation. Yay that worked.

not-poma commented 1 year ago

I was googling for over an hour and am unable to find a way to display multiline explanations under the prompt. zle -R is unable to display line breaks, not to mention color highlighting. I think for now we can resort to instructing GPT to output the entire explanation on a single line. Overall looks good, I'll add some more commits before merging into master.

not-poma commented 1 year ago

I've added some more changes to this PR, now it needs a bit more testing. Fun fact: to add to this PR I had to push into master branch on your fork, GitHub allows to do this.

Psyf commented 1 year ago

Fun fact

Yeah 😆 It warned me that my master branch is unprotected (probably because new convention is main?) I left it such that you could push changes there 😄

some more changes to this PR

Awesome, I looked through them and they all look like good steps!

now it needs a bit more testing

Used it for a bit, looks awesome!

imo, let's merge, the PR is getting a bit big lol

not-poma commented 1 year ago

Did you try to test this version? Looks pretty stable to me, ready to merge

Psyf commented 1 year ago

Did you try to test this version?

Yep, running it for a bit. Stable so far.

Let's merge!