Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
Review effort: Lite
Findings: 2
Open (3)
None => ok((recent.unwrap_or(older), recent.map(|_| older)))mapping is correct but quite… · Newls -loutput.… · Newolderis used both as the name of the expected older-format string and also as the… · New
What changed in this PR
Fixes ls custom time-style parsing so that two-format strings interpret the first line as the non-recent (older) format and the second line as the recent format, while keeping single-format behavior and handling empty halves consistently across --time-style and TIME_STYLE.
Changes:
- Reverse mapping for
+FORMAT1\nFORMAT2soFORMAT1applies to non-recent andFORMAT2to recent files. - Preserve single-format behavior and support empty “older”/“recent” halves.
- Add tests covering option vs
TIME_STYLE, single vs two formats, and empty halves.
| File | Description |
|---|---|
| tests/by-util/test_ls.rs | Updates existing expectations and adds coverage for custom two-format ordering and empty halves via option/env. |
| src/uu/ls/src/config.rs | Adjusts parse_time_style parsing/mapping logic for custom +... time styles to match intended ordering. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
GNU testsuite comparison: |
| match it.next() { | ||
| None => ok((recent, older)), | ||
| Some(_) => Err(LsError::TimeStyleParseError(String::from(field))), | ||
| let older = it.next().unwrap_or_default(); |
There was a problem hiding this comment.
older is passed as the recent format just below, could you rename it to first? it reads wrong otherwise
| } | ||
|
|
||
| #[test] | ||
| fn test_ls_custom_time_style_recent_and_older() { |
There was a problem hiding this comment.
this overlaps quite a bit with test_ls_time_styles, could the new empty-half cases go there instead? thanks
Custom time styles put the non-recent format before the newline and the recent format after it. Reverse the current mapping, retain single-format behavior, and cover empty halves through both the option and TIME_STYLE.
Refs #14727 (the two-format ordering; other strftime cases remain).
Validation: 186 ls tests passed, 1 ignored; Clippy and rustfmt passed.
AI-assisted with Codex.