refactor: reorganize code structure and update dependencies for improved functionality by tamada · Pull Request #44 · tamada/sibling

tamada · 2026-02-11T06:43:51Z

No description provided.

…ved functionality

Copilot

Pull request overview

This pull request refactors the code structure by introducing a new minisib subcommand for shell integration, reorganizing CLI options into a nested structure, and updating dependencies. The main goal appears to be providing a simplified interface for shell scripts to interact with the sibling directory traversal functionality.

Changes:

Added From<&str> and From<String> trait implementations for NexterType enum to enable string-based type conversion
Changed the step parameter type from usize to i32 throughout to support negative step values (for going backwards)
Added new minisib subcommand module with custom argument parsing for shell script integration
Refactored CLI structure by extracting NexterOpts and reorganizing option definitions
Updated rand dependency from 0.9.0 to 0.10.0 and simplified random number generation
Modified bash initialization scripts to use the new minisib command instead of CSV output parsing
Updated Rust edition from 2021 to 2024 in workspace Cargo.toml
Minor code cleanup in printer.rs to remove intermediate variables

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
lib/src/lib.rs	Added From trait implementations for NexterType, changed next_with parameter from usize to i32, simplified Random implementation to use rand::random_range
lib/Cargo.toml	Updated rand dependency from 0.9.0 to 0.10.0
cli/src/minisib.rs	New file implementing minisib subcommand with custom argument parsing for shell integration
cli/src/main.rs	Added minisib module, wrapped set_var calls in unsafe block, updated function signatures to use i32 instead of usize
cli/src/cli.rs	Extracted NexterOpts struct, added minisib subcommand field, added unused strict field
cli/src/printer.rs	Minor refactoring to remove intermediate variable in result_string_impl
assets/init/init.bash	Rewrote __change_directory_with_sibling to use minisib command output instead of CSV parsing
Cargo.toml	Changed edition from 2021 to 2024, removed dist metadata configuration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-11T06:50:39Z

cli/src/minisib.rs

+            } else if let Ok(n) = self.args[2].parse::<i32>() {
+                let step = n;


The variable assignment let step = n; is redundant. The variable n is already the parsed i32 value and can be used directly in the MiniSibOpts::new call. This redundant assignment reduces code clarity.

Copilot · 2026-02-11T06:50:39Z

cli/src/minisib.rs

+        } else {
+            let nexter = NexterType::from(self.args.first().unwrap().as_str());
+            if let Ok(n) = self.args[1].parse::<i32>() {
+                let step = n;
+                let file = Some(self.args[2].as_str().to_string());
+                Ok(MiniSibOpts::new(nexter, step, file))
+            } else if let Ok(n) = self.args[2].parse::<i32>() {
+                let step = n;
+                let file = Some(self.args[1].as_str().to_string());
+                Ok(MiniSibOpts::new(nexter, step, file))
+            } else {
+                Err(Error::Fatal("Invalid step argument".into()))
+            }
+        }


When there are more than 3 arguments, the function only checks if args[1] or args[2] can be parsed as i32, but doesn't handle the case where there are 4 or more arguments. This could lead to ignoring extra arguments silently or accessing args[2] when only checking for 3 arguments. Consider adding validation for the maximum number of arguments and returning an error if too many arguments are provided.

Copilot · 2026-02-11T06:50:40Z

cli/src/cli.rs

+    #[arg(short = 'S', long, help = "strictly check the directory traversing", default_value_t = false)]
+    pub strict: bool,


The strict field is added to the NexterOpts struct but is never used anywhere in the codebase. This appears to be dead code. If this field is intended for future functionality, consider removing it until it's actually implemented, or implement its functionality in this PR.

lib/src/lib.rs

cli/src/minisib.rs

Copilot · 2026-02-11T06:50:40Z

lib/src/lib.rs

-        use rand::Rng;
-        let mut rng = rand::rng();
-        let next = rng.random_range(0..dirs.len());
+        let next = rand::random_range(0..dirs.len());


The function rand::random_range does not exist in the rand crate API (neither in version 0.9.0 nor 0.10.0). The correct API in rand 0.10.0 is rand::rng().random_range(range) or using a thread-local RNG with rand::thread_rng().gen_range(range). The previous implementation using rand::rng() and calling random_range on the RNG was correct. This change will cause a compilation error.

Suggested change

let next = rand::random_range(0..dirs.len());

let next = rand::rng().random_range(0..dirs.len());

Copilot · 2026-02-11T06:50:41Z

cli/src/main.rs

+        unsafe {
+            match level {
+                Error => std::env::set_var("RUST_LOG", "error"),
+                Warn => std::env::set_var("RUST_LOG", "warn"),
+                Info => std::env::set_var("RUST_LOG", "info"),
+                Debug => std::env::set_var("RUST_LOG", "debug"),
+                Trace => std::env::set_var("RUST_LOG", "trace"),
+            };
        }


The use of unsafe block around std::env::set_var is unnecessary and indicates a misunderstanding of when unsafe is required. The std::env::set_var function itself is marked as unsafe in the Rust standard library, but you don't need to wrap the call in an unsafe block - you need to make the entire function unsafe or acknowledge the safety implications. However, wrapping just the match statement doesn't add value since the function call itself is what's unsafe, not the match. The proper approach would be to use unsafe directly on each set_var call, or reconsider whether this pattern is necessary at all.

Suggested change

unsafe {

match level {

Error => std::env::set_var("RUST_LOG", "error"),

Warn => std::env::set_var("RUST_LOG", "warn"),

Info => std::env::set_var("RUST_LOG", "info"),

Debug => std::env::set_var("RUST_LOG", "debug"),

Trace => std::env::set_var("RUST_LOG", "trace"),

};

}

match level {

Error => std::env::set_var("RUST_LOG", "error"),

Warn => std::env::set_var("RUST_LOG", "warn"),

Info => std::env::set_var("RUST_LOG", "info"),

Debug => std::env::set_var("RUST_LOG", "debug"),

Trace => std::env::set_var("RUST_LOG", "trace"),

};

Copilot · 2026-02-11T06:50:41Z

lib/src/lib.rs

+    pub fn next_with(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {
+        nexter.next_with(self, step)


Changing the parameter type from usize to i32 introduces a breaking API change. This affects the public API of the Dirs::next_with method. While this change enables negative step values (which seems intentional for the minisib functionality), it's a breaking change that should be clearly documented and considered carefully. Additionally, the removal of the panic documentation in the comment is concerning - the function can still panic if conversions from usize to i32 fail in the implementation.

Copilot · 2026-02-11T06:50:41Z

cli/src/minisib.rs

+            if let Ok(n) = self.args[1].parse::<i32>() {
+                let step = n;


The variable assignment let step = n; is redundant. The variable n is already the parsed i32 value and can be used directly in the MiniSibOpts::new call. This redundant assignment reduces code clarity.

Copilot · 2026-02-11T06:50:41Z

cli/src/minisib.rs

+            if let Ok(n) = self.args[1].parse::<i32>() {
+                let step = n;


The variable assignment let step = n; is redundant. The variable n is already the parsed i32 value and can be used directly in the MiniSibOpts::new call. This redundant assignment reduces code clarity.

…ved functionality

… usability

…ror handling

…and library

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

assets/init/init.bash

cli/src/printer.rs

Copilot · 2026-02-12T04:30:11Z

cli/src/minisib.rs

+//!     <NUM>: specify the number of times to execute sibling (default: 1)
+//!            -1 means minus one step, -2 means minus two steps, and +2 means plus two steps.
+//! NEXTER_TYPE: next, previous, first, last, random, keep
+//! INPUT_FILE: file containing the list of directories (if not provided, uses current directory


The module-level usage comment is cut off: INPUT_FILE: ... (if not provided, uses current directory is missing a closing parenthesis/ending. Please complete the sentence so the help comment is clear.

Suggested change

//! INPUT_FILE: file containing the list of directories (if not provided, uses current directory

//! INPUT_FILE: file containing the list of directories (if not provided, uses current directory).

cli/src/minisib.rs

lib/tests/skip_non_existent_dir.rs

Copilot · 2026-02-12T04:30:12Z

lib/src/lib.rs

        let path = PathBuf::from(file);
        if !path.exists() {
-            log::error!("Dirs::new_from_file: Not found: {}", path.display());
+            log::error!("Dirs::new_from_file: Not found: {}, pwd: {}", path.display(), std::env::current_dir().unwrap().display());


Dirs::new_from_file_with logs std::env::current_dir().unwrap() on the error path when the list file is missing. If current_dir() fails (e.g., deleted working dir), this will panic and mask the intended Error::NotFound. Consider logging the cwd only when it can be retrieved (or omit it) so the function always returns Error::NotFound without panicking.

Suggested change

log::error!("Dirs::new_from_file: Not found: {}, pwd: {}", path.display(), std::env::current_dir().unwrap().display());

match std::env::current_dir() {

Ok(cwd) => {

log::error!(

"Dirs::new_from_file: Not found: {}, pwd: {}",

path.display(),

cwd.display()

);

}

Err(e) => {

log::error!(

"Dirs::new_from_file: Not found: {}, pwd unavailable: {}",

path.display(),

e

);

}

}

lib/src/lib.rs

Copilot · 2026-02-12T04:30:12Z

lib/src/lib.rs

-    #[must_use]
-    pub fn next_with(&self, nexter: &dyn Nexter, step: usize) -> Option<Dir<'_>> {
-        nexter.next_with(self, i32::try_from(step).unwrap())
+    pub fn next_with(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {


Changing Dirs::next_with to take step: i32 is a breaking change for downstream callers that previously passed usize. If this crate is published, consider keeping the old signature (or adding a new method like next_with_i32 / next_with_signed) and deprecating the old one, or ensure the crate version is bumped according to SemVer.

Suggested change

pub fn next_with(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {

pub fn next_with(&self, nexter: &dyn Nexter, step: usize) -> Option<Dir<'_>> {

let step_i32 = if step > i32::MAX as usize {

i32::MAX

} else {

step as i32

};

nexter.next_with(self, step_i32)

}

/// Get the next directory using the given [`Nexter`] and a signed step.

pub fn next_with_signed(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {

…les for better readability

… resolution

…improve documentation clarity

…in output formatting

…t relative paths

…and enhance documentation for minisib usage

refactor: reorganize code structure and update dependencies for impro…

81348c2

…ved functionality

tamada added this to the v2.0.0 milestone Feb 11, 2026

tamada requested a review from Copilot February 11, 2026 06:43

tamada self-assigned this Feb 11, 2026

tamada added the enhancement New feature or request label Feb 11, 2026

Copilot started reviewing on behalf of tamada February 11, 2026 06:44 View session

Copilot AI reviewed Feb 11, 2026

View reviewed changes

tamada added 3 commits February 12, 2026 10:28

refactor: enhance directory handling and update CLI options for impro…

61c4b3a

…ved functionality

refactor: enhance CLI options and logging initialization for improved…

40997eb

… usability

refactor: improve argument validation in RawOpts parser for better er…

c5cece2

…ror handling

tamada requested a review from Copilot February 12, 2026 04:23

Copilot started reviewing on behalf of tamada February 12, 2026 04:23 View session

refactor: streamline error logging and simplify path handling in CLI …

2612c24

…and library

Copilot AI reviewed Feb 12, 2026

View reviewed changes

tamada added 8 commits February 12, 2026 13:54

refactor: improve code formatting and organization across multiple fi…

2158f22

…les for better readability

refactor: simplify current directory handling in CliOpts initialization

07efe2a

refactor: improve directory change handling and error logging in path…

9db6d8f

… resolution

refactor: enhance error handling for current directory retrieval and …

e567d38

…improve documentation clarity

refactor: rename pathbuf_to_string to path_to_string for consistency …

ab31e40

…in output formatting

refactor: update test assertions for path_to_string to reflect correc…

14652bd

…t relative paths

refactor: improve error handling in current directory initialization …

8541b60

…and enhance documentation for minisib usage

refactor: add unit tests for error display in sibling operations

537c8d0

tamada merged commit 6222914 into release/v2.0.5 Feb 12, 2026
5 checks passed

		} else if let Ok(n) = self.args[2].parse::<i32>() {
		let step = n;

		#[arg(short = 'S', long, help = "strictly check the directory traversing", default_value_t = false)]
		pub strict: bool,

	let next = rand::random_range(0..dirs.len());
	let next = rand::rng().random_range(0..dirs.len());

		pub fn next_with(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {
		nexter.next_with(self, step)

	//! INPUT_FILE: file containing the list of directories (if not provided, uses current directory
	//! INPUT_FILE: file containing the list of directories (if not provided, uses current directory).

-    pub fn next_with(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {
+    pub fn next_with(&self, nexter: &dyn Nexter, step: usize) -> Option<Dir<'_>> {
+        let step_i32 = if step > i32::MAX as usize {
+            i32::MAX
+        } else {
+            step as i32
+        };
+        nexter.next_with(self, step_i32)
+    }
+    /// Get the next directory using the given [`Nexter`] and a signed step.
+    pub fn next_with_signed(&self, nexter: &dyn Nexter, step: i32) -> Option<Dir<'_>> {

Conversation

tamada commented Feb 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants