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

Add commands #59

Merged
merged 11 commits into from
Aug 27, 2023
Merged

Add commands #59

merged 11 commits into from
Aug 27, 2023

Conversation

PCSwingle
Copy link
Member

No description provided.

def apply(self, *args: str) -> None:
pass

# TODO: make more robust way to specify arguments for commands
Copy link
Member Author

Choose a reason for hiding this comment

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

Wanted some outside input on this, but other than this this PR should be good to merge. I have a few ideas for how we could do arguments that I think could be better; for one, we could have an 'argumentvalidator' thing that not only would make it easier to print the argument names (i.e., it could put different brackets around required and optional arguments, as well as including defaults, etc.) but it would also let us print a usage of the command if it's misused. I also think it would be a lot better to include the command arguments in the command initialization (rather than the apply function). Let me know your thoughts on it all!

Copy link
Member

Choose a reason for hiding this comment

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

lets have a call and maybe you can walk me through this

@PCSwingle PCSwingle marked this pull request as ready for review August 14, 2023 00:00
@PCSwingle PCSwingle force-pushed the add-commands branch 2 times, most recently from 942b757 to ea79a6f Compare August 14, 2023 00:09
Copy link
Member

@biobootloader biobootloader left a comment

Choose a reason for hiding this comment

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

Awesome stuff! This is a big step up in functionality we'll be able to have.

I've left some comments, I still need to better understand the prompt_toolkit stuff

mentat/commands.py Outdated Show resolved Hide resolved
mentat/mentat_prompt_session.py Show resolved Hide resolved
def apply(self, *args: str) -> None:
pass

# TODO: make more robust way to specify arguments for commands
Copy link
Member

Choose a reason for hiding this comment

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

lets have a call and maybe you can walk me through this

Copy link
Member

@biobootloader biobootloader left a comment

Choose a reason for hiding this comment

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

🥳

@biobootloader biobootloader merged commit 3374c7b into main Aug 27, 2023
8 checks passed
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.

2 participants