feat: implemented list function for plugins #762
Conversation
ninaagrawal
force-pushed
the
ninaagrawal/plugins
branch
2 times, most recently
from
1f1eb48 to
f43c75c
Compare
Signed-off-by: Nina 'Nino' Agrawal <[email protected]>
ninaagrawal
force-pushed
the
ninaagrawal/plugins
branch
from
f43c75c to
352cd4e
Compare
ninaagrawal
requested review from
patrickjcasey and
j-lanson
and removed request for
patrickjcasey
| #[derive(Debug, Clone)] | ||
| pub enum PluginCacheSort { | ||
| Oldest, | ||
| Alpha, |
There was a problem hiding this comment.
I think we also want a sort for highest-to-lowest version numbers
| HcPluginCacheIterator{ | ||
| wd: Box::new( | ||
| WalkDir::new(path) //builds a new iterator using WalkDir | ||
| .min_depth(3) //makes sure to get the publisher, name, and version folders |
There was a problem hiding this comment.
probably want to add .max_depth(4) so we only get exactly the plugin dirs.
There was a problem hiding this comment.
good point, that should work. We just want to make it so only Paths exactly 3 subdirs down are returned.
| use std::path::{Path, PathBuf, Component}; | ||
| use std::{borrow::Borrow, | ||
| time::{SystemTime,Duration}, | ||
| env | ||
| }; | ||
| use pathbuf::pathbuf; | ||
| use tabled::{Table, Tabled}; | ||
| use walkdir::{DirEntry, WalkDir}; | ||
| use crate::plugin::PluginId; | ||
| use crate::StdResult; | ||
| //use super::super::cli::CliConfig; | ||
| use crate::error::Result; | ||
| use regex::Regex; //used for regex | ||
| use semver::{Version, VersionReq}; | ||
| use crate::cache::repo; |
There was a problem hiding this comment.
please organize the std and crate imports into 2 single use statements like in repo.rs
| ) | ||
| } | ||
| } | ||
| fn path_to_plugin_entry(&self, path: &Path) -> Result<PluginCacheEntry> { //does function need to work when you clone a repo? |
| let components: Vec<String> = path | ||
| .components() | ||
| .filter_map(|component| { | ||
| if let Component::Normal(name) = component { //extracts components seperated by "/" from the path |
There was a problem hiding this comment.
Looks like my spelling isn't as good as I thought :)
| Ok(PluginCacheEntry{ | ||
| publisher:plugin_publisher, | ||
| name:plugin_name, | ||
| version: plugin_version, | ||
| modified: last_modified | ||
| }) | ||
|
|
||
| } | ||
|
|
There was a problem hiding this comment.
Some of this formatting seems a bit weird, can you make sure to run cargo fmt locally to run through the Rust formatter?
| if let Ok(ce) = self.path_to_plugin_entry(e.path()) { | ||
| Some(ce) | ||
| } | ||
| else { | ||
| None | ||
| } |
There was a problem hiding this comment.
this if else structure can be replaced with doing .ok() on the output of self.path_to_plugin_entry(e.path()), which does the same thing. Turns Result<T> into Option<T>, discarding error. Same with the enclosing if else.
| pub path: PathBuf, //path to the root of the plugin cache | ||
| pub entries: Vec<PluginCacheEntry>, //needs to store a vector of PluginEntries |
There was a problem hiding this comment.
Instead of making fields these pub, I would make a special constructor for HcPluginCache marked with #[cfg(test)] so you can only create it that way in the testing code.
|
|
||
| ///lists all the plugin entries. Works the same as the function in repo.rs except for | ||
| ///user can filter plugins by version and publisher | ||
| /// TODO: split into inner function that you can call, outer function can call inner and display on the result |
There was a problem hiding this comment.
TODO comment can be removed now
| let results = HcPluginCache::list_inner(entries, time_sort, None, None, None); | ||
| let expected_output = vec!["affiliation", "activity", "bugs bunny", "difference"]; | ||
| let mut actual_output: Vec<&String> = Vec::new(); | ||
| match results { |
There was a problem hiding this comment.
Instead of doing match results {}, you can just do list_inner(...).unwrap() on line 303 to deal with the error case. We don't care if test cases panic, because it is treated as a test failure.
Also, to create actual_output you can do something like results.map(|x| &x.name) instead of your for-loop. Should cut down a lot on code lines. Don't forget to propagate these changes to the other tests below.
| @@ -64,7 +64,7 @@ impl RepoCacheEntry { | |||
| .unwrap_or("<DISPLAY_ERROR>".to_owned()) | |||
| .to_string() | |||
| } | |||
| fn display_modified(&self) -> String { | |||
| pub fn display_modified(&self) -> String { | |||
There was a problem hiding this comment.
this likely doesn't need to be pub anymore because you copied the implementation wholesale
first implementation of the list function for plugins