-
Notifications
You must be signed in to change notification settings - Fork 672
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
feat[#1540]: Base changes for cli list sessions #1586
base: main
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,33 @@ | |||
use anyhow::Result; | |||
|
|||
pub fn handle_session(verbose: bool, format: String) -> Result<()> { |
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.
rename to handle_session_list
} else { | ||
println!(" {}", id); | ||
} | ||
} |
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.
currently the output looks like this:
❯ goose session list
Available sessions:
20250307_121402
20250307_122909
20250307_125332
20250307_125441
20250307_112710
20250307_121343
20250310_112933
20250307_112732
20250307_124641
i think it'd be nicer to do sth similar to what we have in goose-server routes:
goose/crates/goose-server/src/routes/session.rs
Lines 47 to 79 in fa9bb9e
let sessions = match session::list_sessions() { | |
Ok(sessions) => sessions, | |
Err(e) => { | |
tracing::error!("Failed to list sessions: {:?}", e); | |
return Err(StatusCode::INTERNAL_SERVER_ERROR); | |
} | |
}; | |
let session_infos = sessions | |
.into_iter() | |
.map(|(id, path)| { | |
// Get last modified time as string | |
let modified = path | |
.metadata() | |
.and_then(|m| m.modified()) | |
.map(|time| { | |
chrono::DateTime::<chrono::Utc>::from(time) | |
.format("%Y-%m-%d %H:%M:%S UTC") | |
.to_string() | |
}) | |
.unwrap_or_else(|_| "Unknown".to_string()); | |
// Get session description | |
let metadata = session::read_metadata(&path).expect("Failed to read session metadata"); | |
SessionInfo { | |
id, | |
path: path.to_string_lossy().to_string(), | |
modified, | |
metadata, | |
} | |
}) | |
.collect(); |
then we can print sth like this:
[session_name] - [description] - [modified date]
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.
thanks for contributing. requested few changes on how sessions get listed
Thanks for the review! I tried to keep it simple and figured there would be some changes that needed to be made. I'll get working on the changes ASAP |
I made the requested changes by @salman1993 as well as some code maintainability changes. I held off on the --quiet flag as I want this initial scope to get into main and then we can start adding flags as needed |
I just reused what the ui was using to get the session list and then added a subcommand for "goose session". It checks to see if the subcommand is list and if not runs goose session as normal. I also added an example for arguments such as -v.