Feedback: Code review by golangmalaysia

[allaboutevemirolive]

Click to expand feedback

trees-rs/src/walk/visit.rs

Line 95 in 83639d3

Err(TSimpleError::new(2, "Cannot read target link to symlink"))

Here the internal error is swallowed and replaced by a vague error message. Would be better if it’s retained in your error type for easier debugging.
And Result::map_err might be useful here.

---

trees-rs/src/walk/visit.rs

Line 71 in 83639d3

self.abs.clone()

Can just return references to borrowed values like Option<&Path> and let users clone or to_owned or whatever. Same to other accessors.

---

trees-rs/src/render/buffer.rs

Line 10 in 83639d3

pub trait IntoBranch<W: Write> {

When I see IntoXxx, I would expect a method receiving self not &mut self. Try a better name?

---

https://docs.rs/serde_with/3.8.1/serde_with/struct.DisplayFromStr.html
try this

---

Ivan:

Replace Box with anyhow

---

Not sure if being parallel would be useful, maybe can look into rayon par_bridge.

---

#[derive(Debug)]
pub struct Buffer<W: Write> {
    pub bufwr: io::BufWriter<W>,
}

stdout is buffered, no need to do double buffering.

---

If you want to make it faster, can try to reduce allocations by not allocating within a loop.

2 collect there in one place, maybe can try to put vec in outer function and reuse it to reduce allocations.

This part I think can try to use rayon scope to parallelize stuff and make it faster, but not sure how easy.

---

Also this doing unnecessary work to first serialize it to json and then modify the json. But this is not on the hot path so doesn't really matter.

---

Ang:

You use visitor pattern to iterate through the d(ir)ents.

The terms are orthogonal, not interchangable.

Represent[ing] an operation to be performed on elements of an object structure. Visitor lets you define a new operation without changing the classes of the elements on which it operates.

Yeah, I don't see how that applies to ls-tree. (without over complicating things. You're just doing a treewalk)

---

why you hardcode unix permissions, when you don't need it to render a tree?

allaboutevemirolive: I guess this is where github workflow failed to build for windows :)

---

Dates not in ISO format.

---

Dates not in ISO format.

Tracker

• Error Handling: Preserve Internal Errors for Easier Debugging #9
• Allow accessors to return references to borrowed values #18
• When I see IntoXxx, I would expect a method receiving self not &mut self. Try a better name?
• try this: https://docs.rs/serde_with/3.8.1/serde_with/struct.DisplayFromStr.html
• Replace Box<dyn Error> with anyhow #13
• Not sure if being parallel would be useful, maybe can look into rayon par_bridge.
• stdout is buffered, no need to do double buffering.
• If you want to make it faster, can try to reduce allocations by not allocating within a loop. 2 collect there in one place, maybe can try to put vec in outer function and reuse it to reduce allocations. This part I think can try to use rayon scope to parallelize stuff and make it faster, but not sure how easy.
• Also this doing unnecessary work to first serialize it to json and then modify the json. But this is not on the hot path so doesn't really matter.
• You use visitor pattern to iterate through the d(ir)ents. The terms are orthogonal, not interchangable.
• why you hardcode unix permissions, when you don't need it to render a tree?
• Dates not in ISO format.

[allaboutevemirolive]

See PR #19.