-
Notifications
You must be signed in to change notification settings - Fork 28
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: support markdown plans #79
Conversation
should we get rid of plan.yaml in the |
693ea29
to
652a240
Compare
|
||
|
||
def test_parse_plan_simple(): | ||
plan_str = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious: is this our recommended style for multiline strings? what are the trade offs for this over triple-quoted multiline strings or even keeping the markdown as a fixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to be honest, I did it to pass the ruff checks. Not sure if we've discussed it before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this myself - but would want to see others sure as it kind of is a UX/api change (not that plans are well known). I guess main benefit of yaml is if you had a tool emitting it etc for goose to consume
do we have to use |
yes @lily-de I agree, we should mention it in the docs somehow, because now only the last group of lines that start with “-” is considered the plan |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Some small nits, nothing blocking
src/goose/cli/main.py
Outdated
@click.option("--plan", type=click.Path(exists=True)) | ||
@click.option("-p", "--params", callback=parse_params, help="Parameters in the format param1:value1,param2:value2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can see -p
being confusing for plan and params... maybe -a/--args
instead? up to you tho!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, makes sense! agree
tests/toolkit/test_utils.py
Outdated
"-Run a test\n\n" | ||
"Now actually follow the steps:" | ||
), | ||
"tasks": ["-Step1", "-Step2"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit/maybe not even applicable]should we be stripping -
from the tasks? Just wondering because if someone happens to type in a plan:
-1:do this
-2:do that
then that could be inferred as negative numbers which maybe would change the order of events?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree!
9a1f12d
to
a247a87
Compare
a247a87
to
f55fe29
Compare
* origin/main: docs: add in ollama (block#82) chore: add just command for releasing goose (block#55) feat: support markdown plans (block#79) feat: add version options (block#74) docs: fixing exchange url to public version (block#67) docs: Update CONTRIBUTING.md (block#69) chore: create mkdocs for goose (block#70) docs: fix broken link (block#71) feat: give commands the ability to execute logic (block#63) feat: jira toolkit (block#59) feat: run goose in a docker-style sandbox (block#44)
Supported Markdown plans, made them Jinja-templated, and allowed passing parameters.
The
plan.md
file can be in a free format, but the last group of lines that start with “-” is considered the plan.Some examples:
Example 1:
the plan is:
Example 2:
the plan is:
To start a session with a plan: