Summarise plugins list #2662
Summarise plugins list #2662
Conversation
There was a problem hiding this comment.
This is fine since it's relatively self contained, but I do find it to be slightly hard to follow since we have a bunch of state that mutates as we go along. I would personally find a more verbose solution easier to read, but I won't block this PR from landing if you disagree.
src/commands/plugins.rs
Outdated
| .collect() | ||
| } | ||
|
|
||
| fn latest(mut plugins: Vec<PluginDescriptor>) -> (Option<PluginDescriptor>, Vec<PluginDescriptor>) { |
There was a problem hiding this comment.
This is non-trivial enough that a doc comment explaining what this function would be very helpful.
| fn latest(mut plugins: Vec<PluginDescriptor>) -> (Option<PluginDescriptor>, Vec<PluginDescriptor>) { | ||
| let Ok(versions) = plugins | ||
| .iter() | ||
| .map(|pd| semver::Version::parse(&pd.version)) |
There was a problem hiding this comment.
I'd hate to make this more complicated, but perhaps we want to treat unparseable versions as not possibly being the latest but still try with any versions that do successfully parse.
src/commands/plugins.rs
Outdated
| else { | ||
| return (None, plugins); | ||
| }; | ||
| let Some(latest_ver) = versions.iter().max() else { |
There was a problem hiding this comment.
Would it be possible to do versions.iter().enumerate().max() to get the index of the max item. This index should match the index of the of the plugin in the plugins collection which means you can avoid turning the version into a string and iterating over the plugins collection again.
itowlson
force-pushed
the
plugins-list-only-latest
branch
2 times, most recently
from
ea3686f to
d49a5a0
Compare
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
itowlson
force-pushed
the
plugins-list-only-latest
branch
from
d49a5a0 to
41ec2a8
Compare
Fixes #2638, but:
spin plugins list#2638, I found it awkward because the number after the plugin meant different things (installed or latest) depending on context. So this PR instead proposes a summary where the number by the plugin is always the latest version, and if you have a different version installed, then that's shown in the[installed]bracket. E.g.Note for reviewers: The underlying struct for plugin listing gets a little weird as a result of this, because it can now represent either a single version of a plugin, or an entire plugin (accompanied with latest/installed versions). I considered splitting it out into two different structs, but that would have made the code rather more complicated. Possibly it could be an enum but it didn't feel worth the candle. Definitely open to feedback on this!