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 load/save context commands #534

Merged
merged 21 commits into from
Mar 4, 2024

Conversation

galer7
Copy link
Contributor

@galer7 galer7 commented Feb 24, 2024

resolves #521

@galer7 galer7 marked this pull request as draft February 24, 2024 18:19
@galer7 galer7 changed the title Fix load command to do invalid path check Add load/save context commands Feb 24, 2024
@galer7 galer7 marked this pull request as ready for review February 25, 2024 10:53
Copy link
Member

@jakethekoenig jakethekoenig left a comment

Choose a reason for hiding this comment

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

Thanks so much for doing this. It's looking pretty good. Left a comment on how to fix the windows test.

mentat/code_context.py Outdated Show resolved Hide resolved
mentat/command/commands/load.py Outdated Show resolved Hide resolved
mentat/code_feature.py Outdated Show resolved Hide resolved
mentat/session_stream.py Show resolved Hide resolved
mentat/command/commands/save.py Outdated Show resolved Hide resolved
mentat/command/commands/load.py Outdated Show resolved Hide resolved
@galer7
Copy link
Contributor Author

galer7 commented Feb 28, 2024

Thanks for the review! I'll fix it this week

Copy link
Member

@jakethekoenig jakethekoenig left a comment

Choose a reason for hiding this comment

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

I commented the line which is causing the windows problem. After that change the tests should pass on windows. Both /save and /load work great for me, thanks for doing this. I have one note around error handling. But after those two things are done I think this should be good to merge.

mentat/code_context.py Outdated Show resolved Hide resolved
mentat/command/commands/load.py Outdated Show resolved Hide resolved
Copy link
Member

@jakethekoenig jakethekoenig left a comment

Choose a reason for hiding this comment

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

One more thing. Also sorry about the butler bot. It's a work in progress.

Also it would be great if you updated docs/source/user/commands.rst with the new commands. I need to make that file autogenerated from the code but haven't gotten around to it.

mentat/command/commands/load.py Outdated Show resolved Hide resolved
mentat/command/commands/load.py Outdated Show resolved Hide resolved
@galer7 galer7 requested a review from jakethekoenig March 3, 2024 06:05
@galer7
Copy link
Contributor Author

galer7 commented Mar 3, 2024

Looks like an unlucky run.

I found the same issue here: RobertCraigie/pyright-python#162. Proposed fix is to run brew install npm, so I guess I'll just try adding a actions/setup-node@v4 step in the CI.

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.

Thank you for this! Looks great and works well for me.

I like that the behavior of load is to add to the current context, not replace it. Maybe the help message for the command should be explicit that it does that?

Maybe we should store context files somewhere other than the project root by default? Perhaps a .mentat directory we'd create in the project? That way users could easily add .mentat to their .gitignore. Users could already do this by just running /save .mentat/context_name but that'd be more work. @jakethekoenig what do you think?

@galer7
Copy link
Contributor Author

galer7 commented Mar 4, 2024

I agree, a .mentat folder for each project definitely sounds like a better default


Future idea: a context file per each new git branch, since that would be a new feature & you work on different aspects of your project with each feature

@jakethekoenig
Copy link
Member

I like that the behavior of load is to add to the current context, not replace it. Maybe the help message for the command should be explicit that it does that?

I agree.

Maybe we should store context files somewhere other than the project root by default? Perhaps a .mentat directory we'd create in the project? That way users could easily add .mentat to their .gitignore. Users could already do this by just running /save .mentat/context_name but that'd be more work. @jakethekoenig what do you think?

Context files aren't saved to the project root by default they're saved to ~/.mentat/context.json. I think if the user does specify a name it would be confusing to append anything to it.

@biobootloader
Copy link
Member

Context files aren't saved to the project root by default they're saved to ~/.mentat/context.json. I think if the user does specify a name it would be confusing to append anything to it.

Good point. Lets see how people use this. Maybe one context file is enough and naming it rare

@@ -434,3 +434,29 @@ async def search(
return all_features_sorted
else:
return all_features_sorted[:max_results]

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
simple_dict[str(path.absolute())] = [

This change ensures compatibility with different operating systems and file path formats.

@@ -10,9 +10,11 @@
from .exclude import ExcludeCommand
from .help import HelpCommand
from .include import IncludeCommand
from .load import LoadCommand
Copy link
Contributor

Choose a reason for hiding this comment

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

It's great to see the inclusion of the load and save commands in the __init__.py file, ensuring they are recognized and can be utilized within the application.

Copy link
Contributor

mentatbot bot commented Mar 4, 2024

MENTAT CODE REVIEW IN ACTIVE DEVELOPMENT. Only in use on mentat and internal repos.
Please Reply with feedback.

The addition of load and save commands is a valuable feature, enhancing the flexibility and usability of context management in Mentat. The suggested changes aim to improve error handling and compatibility across different systems, ensuring a smooth user experience. It's also good to see the thoughtful consideration in the command design, such as appending to the current context rather than replacing it. Further discussions on default save locations and context management strategies indicate a forward-thinking approach to feature development.

@biobootloader biobootloader merged commit fe167fb into AbanteAI:main Mar 4, 2024
8 checks passed
@galer7 galer7 deleted the save-load-last-context-used branch March 4, 2024 18:12
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.

save/load context
3 participants