-
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?
Changes from all commits
816f924
1ee294d
671fddf
c7e4a0f
f8c6f6a
b973d2c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -36,19 +36,74 @@ impl List { | |||||||||||||||||||||||||||||||||||||||
| Some(file_name.to_string()) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| let versions = if self.all { | ||||||||||||||||||||||||||||||||||||||||
| let client = super::reqwest_client()?; | ||||||||||||||||||||||||||||||||||||||||
| super::tokio_block_on(super::install::available_releases(&client))?? | ||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||
| paths.cli_bin_dir.installed_versions()? | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Sort versions using semver ordering. Versions that fail to parse are | ||||||||||||||||||||||||||||||||||||||||
| // placed at the end in their original listed. | ||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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(); | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
47
to
59
|
||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // Check for a newer version available on GitHub. | ||||||||||||||||||||||||||||||||||||||||
| let latest = if !self.all { | ||||||||||||||||||||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why only in the
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah because it will be self-evident in that case? |
||||||||||||||||||||||||||||||||||||||||
| let client = super::reqwest_client().ok(); | ||||||||||||||||||||||||||||||||||||||||
| client.and_then(|c| { | ||||||||||||||||||||||||||||||||||||||||
| super::tokio_block_on(super::install::fetch_latest_version(&c)) | ||||||||||||||||||||||||||||||||||||||||
| .ok() | ||||||||||||||||||||||||||||||||||||||||
| .and_then(|r| r.ok()) | ||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||
| None | ||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| // 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()), | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+73
to
+77
|
||||||||||||||||||||||||||||||||||||||||
| // 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.
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.
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?
Uh oh!
There was an error while loading. Please reload this page.
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_installinstead: