60 format option to match buf or rvls

[UmeSiyah]

User description

Hello,

I have renamed the branch to remove the "WIP" prefix, as my work is now ready for review. However, this action closed the existing pull request, so I am opening a new one.

Thank you.

---

PR Type

Enhancement, Documentation

---

Description

• Added customizable output formatting via FormatTemplate

  • Supports default, buf, and nuke formats
  • CLI flag --format to select output style

• Refactored frame parsing logic with new Frames struct

  • Improved regex with named groups for easier manipulation
  • Frames struct holds sequence metadata and utility methods

• Updated tests and documentation for new formatting and parsing features

• Added strfmt as a dependency for flexible string formatting

---

Changes walkthrough 📝

	Relevant files
Enhancement	3 files lib.rs Add output formatting and refactor frame parsing with Frames struct +282/-93 main.rs Add CLI format option and integrate new formatting logic  +14/-1    mega.rs Update benchmarks to use new formatting interface                +6/-5
Dependencies	1 files Cargo.toml Add strfmt dependency for output formatting                            +1/-0
Additional files	4 files aaa.015.tif [link]    aaa.016.tif [link]    aaa.019.tif [link]    aaa.020.tif [link]

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅ 60 - Fully compliant Compliant requirements: Add format option to match buf or rvls output styles Support different output formatting for frame sequences Maintain compatibility with existing functionality

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The error handling in the strfmt function call only prints to stderr but returns an empty string. This could lead to silent failures in production. Some(f) => match strfmt(format, &f.to_hashmap()) { Ok(s) => s, Err(e) => { eprint!("Error formatting string: {}", e); String::new() } }, Potential Performance Issue The frame_list method creates intermediate vectors and performs multiple conversions that could be optimized for better performance. fn frame_list(&self) -> Vec<String>{ let frames_as_isize: Vec<isize> = self.frames.iter() .map(|frame| *frame as isize).collect(); group_continuity(&frames_as_isize).iter().map(|ve| format!("{}:{}", ve[0], ve[ve.len()-1])).collect()

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ Prevent panic on empty vectors Suggestion Impact: The commit implemented the exact suggestion by adding a filter to check for empty vectors before accessing their elements, preventing potential panics code diff: + .map(|frame| *frame as isize).collect(); + group_continuity(&frames_as_isize).iter() + .filter(|ve| !ve.is_empty()) + .map(|ve| format!("{}:{}", ve[0], ve[ve.len()-1])) + .collect() The frame_list method assumes that each vector in the result of group_continuity has at least one element. If an empty vector is encountered, this will cause a panic when accessing ve[0] or ve[ve.len()-1]. Add a check to handle empty vectors safely. src/lib.rs [254-257] fn frame_list(&self) -> Vec<String>{ let frames_as_isize: Vec<isize> = self.frames.iter() .map(|frame| *frame as isize).collect(); - group_continuity(&frames_as_isize).iter().map(|ve| format!("{}:{}", ve[0], ve[ve.len()-1])).collect() + group_continuity(&frames_as_isize).iter() + .filter(|ve| !ve.is_empty()) + .map(|ve| format!("{}:{}", ve[0], ve[ve.len()-1])) + .collect() } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: This suggestion addresses a potential runtime panic by adding a filter to handle empty vectors. This is a critical defensive programming fix that prevents crashes when processing frame sequences.	Medium
	✅ Handle string formatting errors Suggestion Impact: The commit implemented error handling for strfmt() as suggested, replacing the unwrap() call with a match statement that handles formatting errors and provides a fallback format code diff: - strfmt(&format, &f.to_hashmap()).unwrap(), + match strfmt(&format, &f.to_hashmap()) { + Ok(formatted) => formatted, + Err(e) => { + eprintln!("Error formatting path: {}, fallback to default", e); + format!("{name}{sep}{padding}.{ext}@{first_frame}-{last_frame}", + name = f.name, sep = f.sep, padding = "*".repeat(f.padding), ext = f.ext, + first_frame = match f.first_frame() { + Some(first_frame) => first_frame.to_string(), + None => String::from("first_frame") + }, last_frame = match f.last_frame() { + Some(last_frame) => last_frame.to_string(), + None => String::from("last_frames") + } + ) + } + } )); The code unwraps the result of strfmt without handling potential errors, which could cause the program to panic if the format string contains invalid placeholders or if there's any other formatting error. Implement proper error handling instead of using unwrap(). src/lib.rs [532-534] out_frames.push_paths(PathBuf::from( - strfmt(&format, &f.to_hashmap()).unwrap(), + match strfmt(&format, &f.to_hashmap()) { + Ok(formatted) => formatted, + Err(e) => { + eprintln!("Error formatting path: {}", e); + format!("{name}{sep}{padding}.{ext}", + name = f.name, + sep = f.sep, + padding = "*".repeat(f.padding), + ext = f.ext) + } + } )); [Suggestion has been applied] Suggestion importance[1-10]: 8 __ Why: The suggestion replaces an unsafe unwrap() call with proper error handling, preventing potential panics when formatting paths. It also provides a reasonable fallback format, making the code significantly more robust.	Medium
	✅ Improve error handling Suggestion Impact: The commit implemented the suggested error handling approach for frame parsing, but with a slightly different structure. Instead of directly assigning to a frame_num variable, the commit wrapped the error handling in a block and assigned the result directly to frames. code diff: - let frames = vec![frame.parse().unwrap_or(0)]; + let frames = { + let frame_num = match frame.parse::<i32>() { + Ok(num) => num, + Err(e) => { + eprintln!("Error parsing frame number '{}': {}", frame, e); + 0 + } + }; + vec![frame_num] + }; The from_hashmap method doesn't handle potential parse errors properly. If the "frames" value can't be parsed as an integer, it silently defaults to 0, which could lead to incorrect frame sequences. Consider using a more robust error handling approach or at least logging the error. src/lib.rs [214-231] fn from_hashmap(_hashmap: &HashMap<String, String>) -> Self { let name = _hashmap.get("name").unwrap_or(&"None".to_string()).clone(); let sep = _hashmap.get("sep").unwrap_or(&"None".to_string()).clone(); let frame = _hashmap .get("frames") .unwrap_or(&"None".to_string()) .clone(); - let frames = vec![frame.parse().unwrap_or(0)]; + let frame_num = match frame.parse::<i32>() { + Ok(num) => num, + Err(e) => { + eprintln!("Error parsing frame number '{}': {}", frame, e); + 0 + } + }; + let frames = vec![frame_num]; let ext = _hashmap.get("ext").unwrap_or(&"None".to_string()).clone(); let padding = frame.len(); Frames { name, sep, frames, ext, padding, } } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion improves error handling for frame number parsing, replacing a silent fallback to 0 with explicit error reporting. This enhances debugging and makes the code more robust when dealing with invalid input.	Medium
Update

[doubleailes]

@UmeSiyah I have trig the CI/Cd and the performance have drop a lot if you check this CI on your current branch run the benchmark in 3.3210 µs for the Parsing/Mono/1, instead of 1.3858 µs for a Ci running main branch.
Can you try to reduce the drop of performance?

[UmeSiyah]

I can try! However, I haven't optimized Rust code before. Do you have any recommendations for a Rust profiler I could use?

[doubleailes]

Flamegraph is maybe the most common one.

[UmeSiyah]

Hello,

I'm having trouble identifying the source of the slowdowns. Using Flamegraph, I identified and removed an unnecessary hashmap usage (which corresponds to my latest commits), which improved the performance from 6.7953 µs to 5.5447 µs (for the Parsing/Mono/1 on my machine). However, I'm now stuck and struggling to identify further optimizations.

I've added a parse benchmark to test the parse_dir function in isolation, but I haven't noticed any difference (all my tests remain at 1.4 ms).

Here are the two Flamegraphs if you can take a look (I didn't find a tool for comparison). To generate them, I navigate to the samples/small directory and run:
cargo flamegraph --flamechart --inverted -o ../../main.svg

Main:

My branch:

[UmeSiyah]

I tested this morning, and it's strfmt that's slowing things down. I wanted to use this cargo to in the future allow the user to define their own template string to format the output, but due to the performance loss, I'm wondering if I should rewrite otherwise. What do you think?