feat: add history #17
Conversation
- Add ability to recall and reuse previous command options - Implement get_last_options and save_last_options functions - Add 'last' keyword support in show_options
|
hey there. I have been thinking about how to implement something like this. Unfortunately, this doesn't feel like the right implementation to me (I freely admit that it's a personal preference). My goal is to maintain simplicity and ease of use, and add as few commands as possible (i think that I have to think about this a bit more. What i really want are two things:
I'll have to think about this more. Not saying your implementation isn't good, just that it adds an additional keyword for people to remember. I have to ponder it over some more :) |
src/zev/utils.py
Outdated
| def save_last_options(commands: list[Command]): | ||
| app_data_dir = platformdirs.user_data_dir("zev") | ||
| os.makedirs(app_data_dir, exist_ok=True) | ||
| last_options_file = os.path.join(app_data_dir, "last_options.json") |
There was a problem hiding this comment.
see my overarching comment on PR before doing anything here, but i think that if we do go this route, a jsonl file would be more appropriate
There was a problem hiding this comment.
to clarify, jsonl, because i think it's better to keep last few searches (maybe default 100) around
src/zev/main.py
Outdated
| import sys | ||
|
|
||
| from zev.config.setup import run_setup | ||
| from zev.constants import OPENAI_BASE_URL, OPENAI_DEFAULT_MODEL, CONFIG_FILE_NAME | ||
| from zev.constants import OPENAI_BASE_URL, OPENAI_DEFAULT_MODEL, CONFIG_FILE_NAME, EDIT_PROMPT |
There was a problem hiding this comment.
i don't see any changes to zev.constants (where is EDIT_PROMPT coming from?)
There was a problem hiding this comment.
sorry - accidentally left this when deleting the edit prompt feat.
|
Hey @dtnewman, thanks for reviewing the PR. I think it would be best to split the below features into separate PRs, so I'll keep this one for commands history.
I see how introducing a Happy to hear your thoughts if you think there's a cleaner way to approach it. |
|
i've given this some thought. My general take is that i want the vast majority of users to be able to use this by just typing My concern here is that users won't even know that I think the solution is to do what Ollama does here: The add a little filler message with Another thought: when i run Thoughts? p.s. re: splitting into two PRs, 1 for last commands, 1 for incrementing, i think that's reasonable |
…avigating history, move version to constants
|
I completely agree about keeping it extremely simple. Regarding the below is a demo of the changes so far. demo.mp4 |
|
+1000 for including a video I'll take a look in a bit (possibly tomorrow) |
src/zev/history/history.py
Outdated
| class History: | ||
| def __init__(self, max_entries: int = 100): | ||
| self.history_dir = Path.home() / ".zev" | ||
| self.history_file = self.history_dir / "zev_history.jsonl" |
There was a problem hiding this comment.
i think i would just call the file .zevhistory (it can still _contain JSONL, just not be called zev_history.jsonl) rather than creating a new directory for zev
src/zev/history/history.py
Outdated
| dangerous_explanation=cmd_dict.get("dangerous_explanation") | ||
| ) | ||
|
|
||
| def get_history(self) -> Optional[dict[str, OptionsResponse]]: |
There was a problem hiding this comment.
this is so complicated. Why not just read the file in its entirety and return a list?
src/zev/main.py
Outdated
|
|
||
| history_items = list(response_history.keys()) | ||
|
|
||
| while True: |
There was a problem hiding this comment.
i don't like a while true. Doesn't feel right.
src/zev/main.py
Outdated
|
|
||
| selected = cli.display_command_options(response_history[selected_query].commands, f"Commands for '{selected_query}'", history=True) | ||
|
|
||
| if selected == "_back": |
There was a problem hiding this comment.
no need for a back option. Keep it as simple as possible for now. Worst case, user will just rerun the history command.
src/zev/main.py
Outdated
| print(f"zev version: {VERSION}") | ||
| return True | ||
|
|
||
| if command in ("--last", "-l"): |
There was a problem hiding this comment.
i prefer --history and -h, but help is already taken. What about --past
67E6
and -p?
src/zev/ui/cli.py
Outdated
| @@ -0,0 +1,121 @@ | |||
| import questionary | |||
There was a problem hiding this comment.
too much reorganization. For a first-time PR, keep the file structure intact. Don't worry about main.py getting too long. We can tackle that down the road.
347f641 to
91e47c4
Compare
src/zev/main.py
Outdated
| @@ -9,7 +9,8 @@ | |||
| from zev.config.setup import run_setup | |||
| from zev.constants import CONFIG_FILE_NAME | |||
| from zev.llms.llm import get_inference_provider | |||
| from zev.utils import get_env_context, get_input_string | |||
| from zev.utils import get_env_context, show_help, get_input_string | |||
| from zev.history.history import history | |||
There was a problem hiding this comment.
oof... this is a bit much. i think that this kind of tells me that history.py should just be a file at the root level.
src/zev/main.py
Outdated
| f"[red]Could not copy to clipboard: {e} (the clipboard may not work at all if you are running over SSH)[/red]" | ||
| ) | ||
| rprint("[cyan]Here is your command:[/cyan]") | ||
| print(selected.command) |
There was a problem hiding this comment.
why not put all of this history related stuff inside of the History class?
There was a problem hiding this comment.
This ma
BC74
kes sense. Do you think its worth considering a UI class in the future to handle all the console logic like displaying the options, handling user input, copying, etc.?
It feels like bad practice for this History class to have to worry about the questionary logic.
Command History
Historyclass to handle saving and retrieving command history.-pand--pastcommands to display historyArgument Handling
-hand--helpflags to display help documentation.no_prompt()to also check for args.