Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Big change #3

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Big change #3

wants to merge 16 commits into from

Conversation

henke443
Copy link

@henke443 henke443 commented Apr 11, 2023

Note: This change overwrites your recent commit.

You probably want to merge this into a dev branch or something.

  • Made configuration work differently (being saved in memento storage instead).
  • Added ability to change config in the menu.
  • Added more customization options in the context of GPT, like being able to have different models for different prompts and also making it so that you can provided system/assistant/user messages in an array.
  • Added the ability to save the selection for use later on, for example if you want to insert some completion somewhere instead of replacing.
  • Some general opinionated changes, for example prioritizing pnpm over npm because of the extra speed and security, and using prettier because it looks good. I also added .cspell.json.
  • I did some more smaller things, like making the progress more clear when requesting a completion.

Things left to do:

  • Make memento storage sync (super quick)
  • Make fake progress more accurate by using memento storage to store average request time for the current model/brush-type.
  • Add back some functionality and items in the package.json that got lost when fixing the merge conflict I got after your recent update. This includes making your recent Node.js brush changes compatible with this.
  • (fixed) Prompt variable replacement might be buggy, I haven't had time to thoroughly test it yet.
  • Writing tests.

(EDIT) New commits:

  • Made selection API work as expected
  • Added json5 and yaml support
  • Fixed some other bugs
  • Might want to drop pnpm in favor of Yarn because vscode might have bad support somehow 🤔

(EDIT2) More ideas

  • Should add a "Are you sure you want to delete" prompt, both when pressing delete and also when replacing an edit by changing the name
  • Maybe "Uncategorized" should only show up when there are uncategorized items
  • Would be cool to be able to drag and drop brushes into categories
  • Would be cool to be able to add keybindings for any command (including user-created), not sure if this is possible though. One thing we can definitively do is have a fixed number of keybinding slots which the user can then assign in the config or in some other way.
    image
  • I've read in the VSCode documentation (here) that they don't like having tree items as buttons that execute commands. One idea that would solve this is to do what I wrote above and then when you click a tree item the edit screen shows up (and we remove the edit button). You can then assign a keybind inside of the config, or an index referencing one of our command slots. If you want to execute a command without keybinding we can make a command that opens a list (quick-pick) of all the commands like what github copilot labs has, which you can access with a button somewhere and/or CTRL+Shift+P. We could also still keep the ability to run commands from the tree view by replacing the edit button with a play button.
  • Code needs to be cleaned up at some places (lots of outputChannel.appendLine, some unused code, and also some functions that should be split up into smaller functions. Need to make a good brush for this.
  • When adding non-GPT javascript brushes it would be nice to be able to execute other brushes from within a brush.
  • Maybe we should have javascript syntax for javascript brushes? Instead of JSON/YAML I mean. One idea would be to have two different "Add new brush" options (using some context menu or something when the plus sign is pressed) and then that a config is opened for GPT brushes and a .js or .ts is opened for JavaScript brushes. These could then have different icons in the tree view instead of the user assigning icons. We could also of course choose to just have the users write javascript inside of a property of the JSON/YAML and that would work fine too, maybe a bit of premature optimization to fix this now.

…nstead). Added ability to change config in the menu. Added more customization options in the context of GPT, like being able to have different models for different prompts and also making it so that you can provided system/assistant/user messages in an array. Added the ability to save selection for use later on, for example if you want to insert some completion somewhere instead of replacing. I did some more smaller things.
@aKzenT
Copy link
Owner

aKzenT commented Apr 11, 2023

Hey,

thank you very much for the contribution! That looks very nice. I will take a closer look tomorrow. Just a quick question upfront: What is the advantage of using memento storage instead of settings in your opinion? I kind of like being able to see my configured brushes in the settings files.

@henke443
Copy link
Author

henke443 commented Apr 12, 2023

Looking at it again now, there's definitively some bugs with the "variable replacing" but I can fix it soon :). Using node would be a more difficult change for me though. Sorry for making everything in such a big commit I did not do it planning to spend this much effort on it

@henke443
Copy link
Author

henke443 commented Apr 12, 2023

Hey,

thank you very much for the contribution! That looks very nice. I will take a closer look tomorrow. Just a quick question upfront: What is the advantage of using memento storage instead of settings in your opinion? I kind of like being able to see my configured brushes in the settings files.

It's not really about just using memento storage that makes it better, but by the way you edit configs. Now you can click edit a category or brush and then a text editor pops up that contains only the configuration for that item. Same if you create a new brush or category, then a brush or category template pops up. When you save the file it's automatically closed and it's immediately updated in the tree view. This makes it much quicker to edit the configurations. We could also support YAML, having multiline strings would save a lot of headache.

EDIT: I've now added YAML and JSON5 support. I also want to add that the reason I chose Memento over the config file is because it's much easier to use the edit buttons than going into the config manually anyways and Memento is faster than the config. We can still use the config file for general options.

@aKzenT
Copy link
Owner

aKzenT commented Apr 12, 2023

I had a closer look today. I definitely like the easy edit experience, although I'm going to think a bit more about config in general, as I also want to make it easy to have brushes as part of a workspace so that they can be shared in Git etc. I also want the extension to be easily extendable for other "brush providers" in the future, possibly coming from other extensions. This is why I introduced some command abstractions in my last commit. I will have to see how to best combine that with your additions.

I will include the changes piece by piece. That is going to take some time, as I'm a bit busy the next days, but we will get there.

Also I noticed some small bugs during testing:

  • There is a general add button in the tree explorer, but it doesn't do anything.
  • When you edit a brush and change the name, it gets duplicated instead. Not sure, how to avoid it, but I think it is a bit unexpected from a user point of view.
  • When edit a category and change the name, there is also some duplication going on

Also, when installing the application it asked me to install the JSON5 extension. I'm not convinced it is worth requiring that dependency just for JSON5 syntax or if we need to support both JSON and YAML for that matter. YAML seems to be a good choice in that regard.

@henke443
Copy link
Author

henke443 commented Apr 12, 2023

I had a closer look today. I definitely like the easy edit experience, although I'm going to think a bit more about config in general, as I also want to make it easy to have brushes as part of a workspace so that they can be shared in Git etc. I also want the extension to be easily extendable for other "brush providers" in the future, possibly coming from other extensions. This is why I introduced some command abstractions in my last commit. I will have to see how to best combine that with your additions.

I will include the changes piece by piece. That is going to take some time, as I'm a bit busy the next days, but we will get there.

Also I noticed some small bugs during testing:

  • There is a general add button in the tree explorer, but it doesn't do anything.

  • When you edit a brush and change the name, it gets duplicated instead. Not sure, how to avoid it, but I think it is a bit unexpected from a user point of view.

  • When edit a category and change the name, there is also some duplication going on

Also, when installing the application it asked me to install the JSON5 extension. I'm not convinced it is worth requiring that dependency just for JSON5 syntax or if we need to support both JSON and YAML for that matter. YAML seems to be a good choice in that regard.

I will write a better response tomorrow as I really need to sleep but that sounds like a good plan :)

Yes there's definitively some bugs lurking, and also sections of code I definitively want to improve more genrally.

The add brush button in the top right is just straight up not implemented and should be removed.

I agree that we should not have a dependency on the json5 extension, YAML would probably be enough but maybe we can keep the json5 package (not extension) if it doesn't take up much space just for the hell of it 🤷‍♂️

@henke443
Copy link
Author

henke443 commented Apr 13, 2023

I had a closer look today. I definitely like the easy edit experience, although I'm going to think a bit more about config in general, as I also want to make it easy to have brushes as part of a workspace so that they can be shared in Git etc. I also want the extension to be easily extendable for other "brush providers" in the future, possibly coming from other extensions. This is why I introduced some command abstractions in my last commit. I will have to see how to best combine that with your additions.

I will include the changes piece by piece. That is going to take some time, as I'm a bit busy the next days, but we will get there.

Also I noticed some small bugs during testing:

  • There is a general add button in the tree explorer, but it doesn't do anything.
  • When you edit a brush and change the name, it gets duplicated instead. Not sure, how to avoid it, but I think it is a bit unexpected from a user point of view.
  • When edit a category and change the name, there is also some duplication going on

Also, when installing the application it asked me to install the JSON5 extension. I'm not convinced it is worth requiring that dependency just for JSON5 syntax or if we need to support both JSON and YAML for that matter. YAML seems to be a good choice in that regard.

About sharing brushes, we could make an export option to export all of the brushes and also have the ability to import all of them. Could be saved as a compressed file.

I've also made some updates to make the config work better now, which fixes the 3 bugs you mentioned.

As a side note, I really like your idea about using Javascript inside of brushes, something I think would be extra cool is having the ability to execute other brushes from within brushes. This way you can for example parse the GPT result and do loops with GPT prompts to generate more advanced behavior. You could also of course just do some Regex stuff without GPT and that would be super useful also.

PS. I've added some ideas to the PR description TODO list.

PS2. Before progressing further I will try to make my changes compatible with what you've already done. Try to update the repo often if you make changes and I will try to do it within a week or so :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants