Closed lamchau closed 2 weeks 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
keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized
@baxen it's pretty hard to see dim magenta, it'd be friendlier imo if we either keep it the same gray color and not emphasize it with color at all. or alternatively we don't display the file location at all?
keep it dim imo! this info shows every time in case you need it but you mostly don't, so i like it being de-emphasized
@baxen it's pretty hard to see dim magenta, it'd be friendlier imo if we either keep it the same gray color and not emphasize it with color at all. or alternatively we don't display the file location at all?
+1 to not needing tho show the file location at all here
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