-
Notifications
You must be signed in to change notification settings - Fork 32
feat: implement multiple named conversations #40
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
base: main
Are you sure you want to change the base?
Conversation
bytesoverflow
commented
Nov 6, 2024
- Replace single conversation file with a conversations directory structure
- Add support for named conversations via -n/--name flag
- Implement conversation name validation
- Add test coverage for conversation management
|
So, this is my attempt! Critique welcome :) |
a50cc3c to
dd5de60
Compare
4b4deca to
1e27688
Compare
- Replace single conversation file with a conversations directory structure - Add support for named conversations via -n/--name flag - Implement conversation name validation - Add test coverage for conversation management
- Use conversation.toml for storing latest conversation state - Remove unnecessary directory creation in config initialization - Update test coverage
1e27688 to
9f91859
Compare
efugier
left a comment
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.
Hey, good stuff!
After some testing this works nicely you really did a good job!
I am questioning the final API, maybe there should be a shorthand -e "name" instead of -e -n "name" which kinda feels redundant, and -e "name maybe should create the new conversation if it doesn't exist, then we actually lose the need for the -n except for overriding an existing one.
Let me warp my head about what would feel the most natural or use! Happy to hear your thoughts on the matter in the meantime 🙂
|
Thank you, I agree the API feels odd - I think what you're suggesting would be an improvement. |
|
After using I tried modifying the existing The problem is that intent of line 3 is difficult to determine:
I would REALLY love to see this feature added, and I think that a secondary param of EDIT: I just tried out the MR, and it would be nice if this worked: But right now I get an error saying the conversation hasn't started yet. |