-
Notifications
You must be signed in to change notification settings - Fork 686
Sort version output and show upgrade version #4250
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
base: master
Are you sure you want to change the base?
Sort version output and show upgrade version #4250
Conversation
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.
Pull request overview
Fixes spacetime version list output ordering by sorting versions using semver rules and adds a discoverability hint showing the newest available version (with an upgrade command) when newer than the local/current version.
Changes:
- Sorts version list using semver (descending), with a fallback bucket for non-semver strings.
- Adds a GitHub “latest release” lookup and prints an “available” upgrade hint when appropriate.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/update/src/cli/list.rs | Implements semver sorting and prints “available” newest version + upgrade hint. |
| crates/update/src/cli/install.rs | Adds helper to fetch latest release version from the GitHub releases API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Determine whether the latest version is newer than any installed version. | ||
| let newest_installed = sorted_versions.first().and_then(|v| semver::Version::parse(v).ok()); | ||
|
|
||
| let show_latest = match (&latest, &newest_installed) { | ||
| (Some(lat), Some(cur)) if lat > cur => Some(lat.to_string()), |
Copilot
AI
Feb 10, 2026
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.
This compares the GitHub latest version against the newest installed version, but the PR description says the hint should appear when the current version is older than the newest available. If a user has a newer version installed but not selected as current, show_latest will be None and the upgrade hint won’t show even though current is older than latest. Consider parsing current (when present) to semver::Version and comparing latest > current_version (falling back to installed/newest only when current is missing).
| // Determine whether the latest version is newer than any installed version. | |
| let newest_installed = sorted_versions.first().and_then(|v| semver::Version::parse(v).ok()); | |
| let show_latest = match (&latest, &newest_installed) { | |
| (Some(lat), Some(cur)) if lat > cur => Some(lat.to_string()), | |
| // Determine whether the latest version is newer than the current version | |
| // (if set), falling back to the newest installed version otherwise. | |
| let newest_installed = sorted_versions | |
| .first() | |
| .and_then(|v| semver::Version::parse(v).ok()); | |
| let current_version = current | |
| .as_ref() | |
| .and_then(|v| semver::Version::parse(v).ok()); | |
| let show_latest = match (&latest, ¤t_version, &newest_installed) { | |
| // Prefer comparing against the explicitly selected current version. | |
| (Some(lat), Some(cur), _) if lat > cur => Some(lat.to_string()), | |
| // If there is no valid current version, fall back to newest installed. | |
| (Some(lat), None, Some(installed)) if lat > installed => Some(lat.to_string()), |
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.
The PR description is inaccurate in this case. The hint should only show if there is a newer version than any installed version. This is because all installed versions are listed in spacetime version list and therefore discoverable. The main point of showing that there is a new version is that it is otherwise unknown to the user that a new version is available unless they run spacetime upgrade.
| // Sort versions using semver ordering. Versions that fail to parse are | ||
| // placed at the end in alphabetical order. | ||
| let mut parsed: Vec<(semver::Version, String)> = Vec::new(); | ||
| let mut unparsed: Vec<String> = Vec::new(); | ||
| for ver in versions { | ||
| print!("{ver}"); | ||
| if Some(&ver) == current.as_ref() { | ||
| print!(" (current)"); | ||
| match semver::Version::parse(&ver) { | ||
| Ok(sv) => parsed.push((sv, ver)), | ||
| Err(_) => unparsed.push(ver), | ||
| } | ||
| println!(); | ||
| } | ||
| parsed.sort_by(|(a, _), (b, _)| b.cmp(a)); | ||
|
|
||
| let sorted_versions: Vec<String> = parsed.into_iter().map(|(_, s)| s).chain(unparsed).collect(); |
Copilot
AI
Feb 10, 2026
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.
The comment says unparseable versions are placed at the end in alphabetical order, but unparsed is never sorted—it's appended in the original iteration order. Either sort unparsed before chaining (e.g., unparsed.sort()), or adjust the comment to match the actual behavior.
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.
Updated the comment
| .json() | ||
| .await?; | ||
| release.version().context("Could not parse latest version number") | ||
| } |
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.
nit: instead of adding this function, I think we could instead factor this out of download_and_install instead:
pub(super) async fn fetch_release(
client: &reqwest::Client,
version: Option<semver::Version>,
) -> anyhow::Result<Release> {
let releases_url = releases_url();
let url = match &version {
Some(version) => format!("{releases_url}/tags/v{version}"),
None => [&*releases_url, "/latest"].concat(),
};
let release = client
.get(url)
.send()
.await?
.error_for_status()
.map_err(|e| {
if e.status() == Some(reqwest::StatusCode::NOT_FOUND) {
if let Some(version) = &version {
return anyhow::anyhow!(e).context(format!("No release found for version {version}"));
}
}
anyhow::anyhow!(e).context("Could not fetch release info")
})?
.json()
.await?;
Ok(release)
}| }; | ||
|
|
||
| // Sort versions using semver ordering. Versions that fail to parse are | ||
| // placed at the end in their original listed. |
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.
maybe a dumb question, but why would a version fail to parse?
| let sorted_versions: Vec<String> = parsed.into_iter().map(|(_, s)| s).chain(unparsed).collect(); | ||
|
|
||
| // Check for a newer version available on GitHub. | ||
| let latest = if !self.all { |
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.
why only in the !self.all case? I would expect this hint to happen regardless of whether we run with --all?
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.
ah because it will be self-evident in that case?
| let newest_installed = sorted_versions.first().and_then(|v| semver::Version::parse(v).ok()); | ||
|
|
||
| let show_latest = match (&latest, &newest_installed) { | ||
| (Some(lat), Some(cur)) if lat > cur => Some(lat.to_string()), |
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.
I think if the current version doesn't parse, we should probably the hint to upgrade them to the latest version.
| if let Some(ref new_ver) = show_latest { | ||
| println!("{} {}", new_ver, "(available - run `spacetime version upgrade`)"); | ||
| } | ||
| } |
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.
this does seem weird if the current version isn't the latest
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.
how about instead of trying to show the latest version "inline", we just print a message at the top that says something like "version xyz is available, run spacetime version upgrade to update!" and then print the list as normal?
Description of Changes
The output of
spacetime version listis sorted in alphabetical order, which is not the correct semver order.This fixes the order printed, by sorting correctly.
Example before:
After:
Additionally, for discoverability when running spacetime version list, if the current version is older than the newest available version, the newest version is shown with a hint on how to upgrade to it:
API and ABI breaking changes
Not an API or ABI breaking change
Expected complexity level and risk
1
Testing
Ran
spacetime version listandspacetime version list --allwith the new code and confirmed that the result was correctly sorted.I additionally tested that if the latest available version was not installed that it would show with the upgrade hint command.