Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

parkeregli
Copy link

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.

@@ -0,0 +1,33 @@
use anyhow::Result;

pub fn handle_session(verbose: bool, format: String) -> Result<()> {
Copy link
Collaborator

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);
}
}
Copy link
Collaborator

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:

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]

Copy link
Collaborator

@salman1993 salman1993 left a 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

@parkeregli
Copy link
Author

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

@parkeregli
Copy link
Author

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

@parkeregli parkeregli requested a review from salman1993 March 10, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants