Release/v2.0.1#38
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements the Release v2.0.1 of the sibling project, performing a major code refactoring to separate concerns between library and CLI functionality. The main changes include restructuring the codebase into a workspace with separate library and CLI packages, modernizing the API design, and updating project configuration.
- Restructured the project into a Rust workspace with separate
libandclipackages - Modernized the API by removing mutable state and improving type safety
- Updated build system and removed Windows installer configuration
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wix/main.wxs | Removed Windows installer configuration file |
| src/*.rs | Removed original source files (moved to lib/cli structure) |
| lib/src/lib.rs | New library implementation with improved API design |
| cli/src/*.rs | New CLI implementation using the library |
| Cargo.toml | Converted to workspace configuration |
| build.rs | Removed build script (functionality moved to CLI) |
| let mut rng = rand::rng(); | ||
| let next = rng.random_range(0..dirs.dirs.len()) as usize; |
There was a problem hiding this comment.
The rand::rng() function may not exist in rand 0.9.0. Use rand::thread_rng() instead for thread-local random number generation.
| let mut rng = rand::rng(); | |
| let next = rng.random_range(0..dirs.dirs.len()) as usize; | |
| let mut rng = rand::thread_rng(); | |
| let next = rng.gen_range(0..dirs.dirs.len()) as usize; |
| let mut rng = rand::rng(); | ||
| let next = rng.random_range(0..dirs.dirs.len()) as usize; |
There was a problem hiding this comment.
The random_range method doesn't exist in rand 0.9.0. Use gen_range(0..dirs.dirs.len()) instead.
| let mut rng = rand::rng(); | |
| let next = rng.random_range(0..dirs.dirs.len()) as usize; | |
| let mut rng = rand::thread_rng(); | |
| let next = rng.gen_range(0..dirs.dirs.len()); |
| pub fn build_nexter(nexter_type: NexterType) -> Box<dyn Nexter> { | ||
| match nexter_type { | ||
| NexterType::First => Box::new(First {}), | ||
| NexterType::Last => Box::new(Last {}), | ||
| NexterType::Previous => Box::new(Previous {}), | ||
| NexterType::Next => Box::new(Next {}), | ||
| NexterType::Random => Box::new(Random {}), | ||
| NexterType::Keep => Box::new(Keep {}), | ||
| } | ||
| } |
There was a problem hiding this comment.
The build_nexter function duplicates the logic from NexterFactory::build. Consider removing this function and using only the factory method to avoid code duplication.
| pub fn build_nexter(nexter_type: NexterType) -> Box<dyn Nexter> { | |
| match nexter_type { | |
| NexterType::First => Box::new(First {}), | |
| NexterType::Last => Box::new(Last {}), | |
| NexterType::Previous => Box::new(Previous {}), | |
| NexterType::Next => Box::new(Next {}), | |
| NexterType::Random => Box::new(Random {}), | |
| NexterType::Keep => Box::new(Keep {}), | |
| } | |
| } | |
| // Removed the `build_nexter` function as its logic is already implemented in `NexterFactory::build`. |
| fn find_current_dir_index(dirs: &[PathBuf]) -> usize { | ||
| if let Ok(pwd) = std::env::current_dir() { | ||
| let cwd = PathBuf::from("."); | ||
| if let Some(pos) = dirs | ||
| .iter() | ||
| .position(|dir| dir == &cwd || pwd.ends_with(dir)) | ||
| { | ||
| return pos; | ||
| } |
There was a problem hiding this comment.
The function signature changed from &Vec<PathBuf> to &[PathBuf] which is good, but the function calls std::env::current_dir() on every call. Consider caching this value if the function is called multiple times.
| fn find_current_dir_index(dirs: &[PathBuf]) -> usize { | |
| if let Ok(pwd) = std::env::current_dir() { | |
| let cwd = PathBuf::from("."); | |
| if let Some(pos) = dirs | |
| .iter() | |
| .position(|dir| dir == &cwd || pwd.ends_with(dir)) | |
| { | |
| return pos; | |
| } | |
| fn find_current_dir_index(dirs: &[PathBuf], current_dir: &PathBuf) -> usize { | |
| let cwd = PathBuf::from("."); | |
| if let Some(pos) = dirs | |
| .iter() | |
| .position(|dir| dir == &cwd || current_dir.ends_with(dir)) | |
| { | |
| return pos; |
| fn perform_impl( | ||
| dirs: sibling::Dirs, | ||
| nexter: &dyn Nexter, | ||
| step: usize, |
There was a problem hiding this comment.
The step parameter changed from i32 to usize, but the Nexter trait still expects i32. This type mismatch requires casting and could cause issues with negative values that were previously valid.
| step: usize, | |
| step: i32, |
No description provided.