BuilderIO / ai-shell

A CLI that converts natural language to shell commands.
MIT License
4.04k stars 257 forks source link

Refactoring opportunity, maybe? 🤷🏾‍♂️ #61

Closed Rafaelcv7 closed 1 year ago

Rafaelcv7 commented 1 year ago

Hey I was thinking that on this piece of the code on the prompt.ts we can refactor it. The value property on each option could be an anonymous function that do the actions specified on the if sequence. That way we can get rid of the "const x = answer === y" part and the different IF calculations. Then you will only need to call answer() to execute the action.

Unless I'm not picking up something it seems to me like a good idea, and I could implement it if you want. Please let me know if Im wrong or if you want me to do it. 😄

const answer = await p.select({
    message: nonEmptyScript ? 'Run this script?' : 'Revise this script?',
    options: [
      ...(nonEmptyScript
        ? [
            { label: '✅ Yes', value: 'yes', hint: 'Lets go!' },
            {
              label: '📝 Edit',
              value: 'edit',
              hint: 'Make some adjustments before running',
            },
          ]
        : []),
      {
        label: '🔁 Revise',
        value: 'revise',
        hint: 'Give feedback via prompt and get a new result',
      },
      {
        label: '📋 Copy',
        value: 'copy',
        hint: 'Copy the generated script to your clipboard',
      },
      { label: '❌ Cancel', value: 'cancel', hint: 'Exit the program' },
    ],
  });

  const confirmed = answer === 'yes';
  const cancel = answer === 'cancel';
  const revisePrompt = answer === 'revise';
  const copy = answer === 'copy';
  const edit = answer === 'edit';

  if (revisePrompt) {
    await revisionFlow(script, key, apiEndpoint, silentMode);
  } else if (confirmed) {
    await runScript(script);
  } else if (cancel) {
    p.cancel('Goodbye!');
    process.exit(0);
  } else if (copy) {
    await clipboardy.write(script);
    p.outro('Copied to clipboard!');
  } else if (edit) {
    const newScript = await p.text({
      message: 'you can edit script here:',
      initialValue: script,
    });
    if (!p.isCancel(newScript)) {
      await runScript(newScript);
    }
  }
steve8708 commented 1 year ago

Works for me! Bonus point if you can add a unit test to verify the behavior is the same before and after

pcheng17 commented 1 year ago

Somewhat related - I'm curious if it's possible add in the ability to select an option using a letter? So instead of hitting the down arrow to get to "Cancel," for example, one could just hit "X" and it would immediately select the cancel option.

Rafaelcv7 commented 1 year ago

Works for me! Bonus point if you can add a unit test to verify the behavior is the same before and after

Nice! I would then implement it on this next week is possible for me. I would try to set the Unit tests too.

Somewhat related - I'm curious if it's possible add in the ability to select an option using a letter? So instead of hitting the down arrow to get to "Cancel," for example, one could just hit "X" and it would immediately select the cancel option.

Mmm, this is interesting, although I like arrow keys as navigation on the terminal menus. I would see if their is a good way to implement a relation of keyboard letters to each command also.

pcheng17 commented 1 year ago

Mmm, this is interesting, although I like arrow keys as navigation on the terminal menus. I would see if their is a good way to implement a relation of keyboard letters to each command also.

Oh for sure, keep the arrow key navigation because I think that's the natural thing most people will reach for. But if possible, I think having the option to set a letter as a shortcut for the options might be nice/convenient. :)

Rafaelcv7 commented 1 year ago

Closing Issue because a Pull Request has already been merged addressing this issue. PR: #63