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

feat: add execute method to commands #17

Closed
wants to merge 14 commits into from

Conversation

lukealvoeiro
Copy link
Contributor

@lukealvoeiro lukealvoeiro commented Aug 27, 2024

About

Before this PR, commands would lexed in the following way:

/file:)("asf) asdf"


After this PR, commands are lexed as so:

  • Improves lexing of commands, recognizing command values within quote marks to allow folks to add plenty of spaces
  • Implements the execute portion of the command
  • Adds the surrounding context to the command, in case the command needs it.

@lukealvoeiro lukealvoeiro changed the title tips for ticket implementing .execute(...) command Aug 27, 2024
@lukealvoeiro lukealvoeiro changed the title implementing .execute(...) command implementing .execute(...) method for commands Aug 27, 2024
* main:
  fix: typo in exchange method `rewind` (#54)
  fix: remove unsafe pop of messages (#47)
  chore: Update LICENSE (#53)
  chore(docs): update is_dangerous_command method description (#48)
  refactor: improve safety rails speed and prompt (#45)
  feat: make goosehints jinja templated (#43)
  ci: enforce PR title follows conventional commit (#14)
  feat: show available toolkits (#37)
  adding in ability to provide per repo hints (#32)
  Apply ruff and add to CI (#40)
  added some regex based checks for dangerous commands (#38)
  chore: Update publish github workflow to check package versions before publishing (#19)
  chore: upgrade ai-exchange dependency (#36)
  fix: resuming sessions (#35)
  feat: upgrade `ai-exchange` to version `0.8.3` and fix tests (#34)
  fix: export metadata.plugins export should have valid module (#30)
  fix (#24)
  link to vs code extension (#20)
  Enable cli options for plugin (#22)
  Modified the readme to be more friendly to new users (#16)
@lukealvoeiro lukealvoeiro changed the title implementing .execute(...) method for commands feat: add execute method to commands Sep 11, 2024
@lukealvoeiro lukealvoeiro marked this pull request as ready for review September 11, 2024 17:23


def create_prompt() -> PromptSession:
def create_prompt(commands: dict[str, Command]) -> PromptSession:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is an old function but can we get a docstring since it's part of your PR 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for sure, good call!

@staticmethod
def create_prompt_session() -> "GoosePromptSession":
return GoosePromptSession(create_prompt())
def get_message_after_commands(self, message: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments to make the flow of logic clearer to the uninitiated but feel free to ignore!

@lifeizhou-ap
Copy link
Collaborator

lifeizhou-ap commented Sep 11, 2024

I am curious whether there is any real use case to use the command execute? If not, Shall we wait to implement this until we have the real requirements? In this case the implementation will fit better for the requirement.

It would be good to have a clear purpose while we adding code as code maintenance could be a cost.

@lukealvoeiro
Copy link
Contributor Author

lukealvoeiro commented Sep 11, 2024

@lifeizhou-ap we have the quorum toolkit which will be using this, as well as the file command. given this, that's why the ticket has a high priority on our internal Linear board.

it's also a bit of a catch-22, where if we don't give engineers the ability to run commands, we won't see additional use-cases for it.

@jtorreggiani
Copy link
Collaborator

Sorry for automated comments on this pull request. Goose used the current directory instead of the plugins repo to send these comments.

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.

4 participants