Skip to content

Conversation

@doubleailes
Copy link
Collaborator

@doubleailes doubleailes commented Sep 23, 2025

PR Type

Enhancement


Description

  • Add is_empty method to Paths collection

  • Simplify filter and regex extraction logic

  • Improve code readability with clippy suggestions

  • Update documentation examples formatting


Diagram Walkthrough

flowchart LR
  A["Paths struct"] --> B["Add is_empty method"]
  C["Filter logic"] --> D["Simplify with map"]
  E["Range checks"] --> F["Use contains syntax"]
  G["Documentation"] --> H["Clean up examples"]
Loading

File Walkthrough

Relevant files
Enhancement
lib.rs
Simplify logic and improve documentation                                 

src/lib.rs

  • Simplify filter_map logic using map instead of and_then
  • Replace range checks with contains for better readability
  • Remove unnecessary string reference in regex capture
  • Clean up documentation examples by removing function wrappers
  • Use PathBuf::from directly instead of closure
+15/-18 
paths.rs
Add is_empty method to Paths                                                         

src/paths.rs

  • Add new is_empty method to check if Paths collection is empty
  • Include comprehensive documentation with examples
  • Method delegates to underlying Vec's is_empty implementation
+21/-0   

@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Panic

In extended_listing, indexing value[0] assumes non-empty vectors; if parse_result ever yields an empty vec for a key, this will panic. Prefer checking is_empty() or using first().

if value[0] == "None" && value.len() == 1 {
    out_frames.push_metadata(get_exr_metada(&root_path, &key));
    out_frames.push_paths(key.into());
} else {
    let to = value.first().unwrap();
    let from = String::from_utf8(vec![b'*'; to.len()]).unwrap();
    let new_path = &key.replace(&from, to);
Borrow Lifetime

new_path is a borrowed &String from key.replace; it’s used immediately which is fine, but ensure no accidental use beyond this scope or moves in future refactors. Optionally assign the owned String directly to avoid confusion.

let to = value.first().unwrap();
let from = String::from_utf8(vec![b'*'; to.len()]).unwrap();
let new_path = &key.replace(&from, to);
out_frames.push_metadata(get_exr_metada(&root_path, new_path));
out_frames.push_paths(format!("{}@{}", key, create_frame_string(value)).into());
Doc Example Accuracy

Updated examples remove main and show top-level statements; ensure doctests compile and have required imports (e.g., Paths, Join trait) and that parse_dir path exists or is mocked to avoid flaky doctests.

//! ```rust
//! use framels::{basic_listing, extended_listing, parse_dir, paths::{Paths,Join}, recursive_dir};
//!
//! 
//! // Perform directory listing
//! let in_paths: Paths = parse_dir("./samples/small");
//!
//! // Generate results based on arguments
//! let results: String = basic_listing(in_paths, false).get_paths().join("\n");
//!
//! println!("{}", results)
//! ```

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements Clippy suggestions to improve code quality and style consistency. The changes focus on idiomatic Rust patterns, eliminating unnecessary code, and improving readability.

  • Adds is_empty() method to the Paths struct for better API completeness
  • Applies various Clippy suggestions including simplifying closures, using range patterns, and removing redundant code
  • Cleans up documentation examples by removing unnecessary function wrappers

Reviewed Changes

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

File Description
src/paths.rs Adds is_empty() method with comprehensive documentation
src/lib.rs Applies multiple Clippy suggestions for cleaner, more idiomatic Rust code

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

.sort(true)
.into_iter()
.filter_map(|entry| entry.ok().and_then(|e| Some(e.path())))
.filter_map(|entry| entry.ok().map(|e| e.path()))
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map operation returns &Path but the collection expects PathBuf. This will cause a type mismatch. Change to .filter_map(|entry| entry.ok().map(|e| e.path().to_path_buf())) to convert &Path to PathBuf.

Suggested change
.filter_map(|entry| entry.ok().map(|e| e.path()))
.filter_map(|entry| entry.ok().map(|e| e.path().to_path_buf()))

Copilot uses AI. Check for mistakes.
.expect("Can't compile regex");
}
let result_caps: Option<Captures> = RE_FLS.captures(&x);
let result_caps: Option<Captures> = RE_FLS.captures(x);
Copy link

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The explicit type annotation Option<Captures> is redundant since the compiler can infer it from the captures() method return type. Consider removing it for cleaner code.

Suggested change
let result_caps: Option<Captures> = RE_FLS.captures(x);
let result_caps = RE_FLS.captures(x);

Copilot uses AI. Check for mistakes.
@qodo-code-review
Copy link

qodo-code-review bot commented Sep 23, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant