Open lamchau opened 4 days ago
Hi @lamchau,
Nice one!
I noticed that the session.py file is getting bigger. Just wondering whether it is better to separate some of these logics to another component or file?
also I feel that with your change, we may not require session resume
as user can resume
the session via session start
. WDYT?
@lifeizhou-ap maybe after we get to 500 lines or so unless you have another suggestion?
30% or so of the Session
class is text strings (split lines) and docstrings - with code folding it feels digestible. i'm worried if we start to make it to abstract it requires more context switching/modification across files
[formatted for clarity]
@lifeizhou-ap possibly! but the challenge lies in how it's invoked in this project and internally as a subcommand and how it's used via pipx
xx goose session start <tab>
doesn't autocomplete session names
xx
doesn't have shell completions so click
can't autocomplete on <tab>
we would need to add shell completions to xx
(in golang
)xx goose session list
(get top)xx goose session start <session_name>
r
to resumewith pipx
it would still require a conflict for a resume to happen (e.g. goose session start
) creates a generated session each time, so that requires the user to use <tab>
to autocomplete the latest one
goose session start <tab>
r
to resumeif we keep the command xx goose session resume
and goose session resume
it is only 1 step - this convenience is essentially "free" and saves a few steps for the user.
anyhoo, those are my thoughts/biases - we should go with whatever flow makes the most sense. since it's a product decision, maybe anna can make the call?
@lifeizhou-ap maybe after we get to 500 lines or so unless you have another suggestion?
30% or so of the
Session
class is text strings (split lines) and docstrings - with code folding it feels digestible. i'm worried if we start to make it to abstract it requires more context switching/modification across files
I see. Personally I prefer small files for readability and maintenance. However, I understand your concern and we can refactor in the future when it reaches the point for refactoring.
One suggestion: Maybe extract the code into a function?
# prevents cluttering the `sessions` with empty files, which
# can be confusing when resuming a session
if is_empty_session(self.session_file_path):
try:
self.session_file_path.unlink()
except FileNotFoundError:
pass
except Exception as e:
raise Exception(f"error deleting empty session file: {e}")
Hey @baxen I am happy with this PR (only with a small suggestion). Would you like to review it?
@lifeizhou-ap sure thing, extracted and reordered them here a1c8c82
added a fix/workaround for
rich
color handlingswitched
[dim]
to[bold]
for dark terminalsadded a cleanup for empty sessions: edge case when a user aborts early - without it empty sessions are generated so we're unable to find the "right" last valid session to resume
added a prompt menu for overwrites: added a check for existing session files and give a user an opportunity to recover before they take a destructive action
note: the image shows both the empty session file (
goose session list
doesn't containabcd.jsonl
) and what the menu ux looks like for a session conflict