Conversation
joknarf
force-pushed
the
classify_target
branch
from
9f9989d to
f11d4e6
Compare
joknarf
force-pushed
the
classify_target
branch
from
77108f2 to
c9b81dc
Compare
Alonely0
left a comment
Alonely0
left a comment
There was a problem hiding this comment.
Looks fine, but there are a lot of removed comments (vibecode?) and unnecessary performance hits.
| } | ||
|
|
||
| fn indicator_char(path: &PathData, style: IndicatorStyle) -> Option<char> { | ||
| let sym = classify_file(path); |
There was a problem hiding this comment.
The classify_files() function is now called unconditionally, but beforehand it was contingent on config.indicator_style != IndicatorStyle::None. I'm concerned this is going to negatively affect performance since it has to stat files. Maybe the best solution is to remove the IndicatorStyle::None branch and change all occurrences of IndicatorStyle to Option<IndicatorStyle>, then do a let-else.
There was a problem hiding this comment.
PD: please keep the old comments on the code you refactored to this new function. Did you vibecode this? Don't take it the wrong way; this is just a usual telltale.
| None => target_path.clone(), | ||
| } | ||
| } else { | ||
| target_path.clone() |
There was a problem hiding this comment.
Please do not clone here; it is not necessary. Use references, like the previous code that you have replaced. This was fixed in edfe26c.
| ), | ||
| ); | ||
| } else { | ||
| name.push(escaped_target); |
There was a problem hiding this comment.
Please keep comments. Previously in L804-805.
| false, | ||
| ); | ||
|
|
||
| let mut target_display = escaped_target.clone(); |
There was a problem hiding this comment.
Please do not clone here. You should refactor the immediate assignment inside the if to be the expression of the let definition, like let var = if ... { ... } else { ... };. Alternatively, use references or a Cow if you have to.
| false, | ||
| ); | ||
|
|
||
| // Check if the target actually needs coloring |
There was a problem hiding this comment.
Keep this comment if you can.
| ); | ||
|
|
||
| if style.is_some() { | ||
| // Only apply coloring if there's actually a style |
There was a problem hiding this comment.
Keep this comment if you can.
| is_wrap(name.len()), | ||
| )); | ||
| } else { | ||
| // For regular files with no coloring, just use plain text |
There was a problem hiding this comment.
Once/if you apply the other requested changes, consider keeping this comment.
2 participants
Fix for #11542
gnu output incompatibility with
--classifyon symlink targetsbefore:
after (gnu identical output):