Include the underlying reason in invalid --changed-before/-within date errors#2054
Include the underlying reason in invalid --changed-before/-within date errors#2054momomuchu wants to merge 3 commits into
Conversation
…hin dates Previously an invalid date like 2025-11-31 (November has 30 days) was rejected with a generic 'is not a valid date or duration' message, discarding the actual reason. Surface the inner parser error instead, e.g. 'day ... is invalid, must be in range 1..=30', so the user can tell it's an out-of-range calendar date rather than a formatting issue. Fixes sharkdp#2053
| /// source of confusing failures, e.g. `2025-11-31` which is not a valid | ||
| /// calendar date) instead of silently discarding it, so callers can tell | ||
| /// the user *why* their input was rejected. | ||
| fn from_str(s: &str) -> Result<SystemTime, String> { |
There was a problem hiding this comment.
Why is the error type a string, and not an anyhow error or jiff::Error?
| } | ||
|
|
||
| #[test] | ||
| fn invalid_calendar_date_error_includes_inner_reason() { |
There was a problem hiding this comment.
This test seems a little brittle. If the jiff error messages change, then this test could fail on a version update.
| // parser gives the most useful reason for calendar-date- | ||
| // shaped input (the common case for this kind of mistake), | ||
| // so surface that instead of a generic message. | ||
| Err(datetime_err.to_string()) |
There was a problem hiding this comment.
Is this error message always user-friendly, or are there cases where this could expose implementation details.
| { | ||
| return Ok(UNIX_EPOCH + Duration::from_secs(timestamp_secs)); | ||
| } | ||
| // None of the supported formats matched. The `DateTime` |
There was a problem hiding this comment.
This could however be more confusing if the user meant to use a span, or timestamp.
Addresses review comments on sharkdp#2054. TimeFilter::from_str/before/after now return anyhow::Result instead of Result<_, String>, matching the error convention used elsewhere in the crate (e.g. walk.rs). Call sites in main.rs already compile unchanged since anyhow::Error's Display shows the same top-level message a String did. Also relax the calendar-date test: instead of asserting the message contains "day" (coupled to jiff's exact wording), it now checks the message is non-empty and differs from the message for an unrelated malformed input, which still proves the inner reason is surfaced without breaking on a jiff wording change.
The previous version only checked that two invalid inputs produce different messages, which still passes if the code drops the real parse reason but keeps echoing the raw input. Compare two dates that are invalid for the same reason (day 31 in a 30-day month) against one invalid for a different reason (month 13), and require the same-reason pair to share substantially more text. Still avoids asserting on jiff's exact wording.
|
Thanks, good points. 1. Error type: switched from_str/before/after to anyhow::Result - the rest of the crate already uses anyhow (main.rs, walk.rs), String was the outlier. main.rs needed no change since anyhow::Error's Display gives the same message; fd is bin-only so no API concern. 2. Test brittleness: reworked it so it no longer matches jiff's literal wording - it checks that two dates invalid for the same reason share more of the message than a date invalid for a different reason (computed common-substring length), so it survives a jiff reword but still proves the real parse reason is surfaced, not just the echoed input. 3. Error message exposure: no sensitive data - the appended text only echoes the user's own input plus generic range wording (must be in range 1..=30), no paths or secrets. 4. Span/timestamp confusion: fair - a malformed span/timestamp currently shows the calendar-date parser reason, which can read off-topic. I can gate the appended reason to only show when the input looks date-shaped, with a neutral fallback otherwise. Want that in this PR or a follow-up? |
2 participants
fd --changed-before=2025-11-31(an invalid calendar date) prints a generic "'...' is not a valid date or duration" and hides the actual reason.TimeFilter::from_strreturnedOption<SystemTime>via.ok()?, discarding the underlying parse errors from all four format attempts (span, timestamp, datetime,@timestamp). It now returnsResult<_, String>and, on total failure, surfaces theDateTimeparser's error, which is the most relevant one for calendar-date-shaped input.Before:
'2025-11-31' is not a valid date or duration. See 'fd --help'.After:
'2025-11-31' is not a valid date or duration: parsed date is not valid: parameter 'day' for '2025-11' is invalid, must be in range 1..=30. See 'fd --help'.Added a unit test asserting the error for an out-of-range calendar date includes the reason.
cargo test, clippy (-Dwarnings) and fmt clean. CHANGELOG entry added.Note on non-calendar input: the surfaced reason is always the
DateTime(calendar) parser's error. That is precise for calendar-shaped mistakes (2025-13-01,2025-02-30, ...), but for input that is not date-shaped at all (e.g. a mistyped duration like5xyz) the reason reads asfailed to parse year in date, which is a bit off-topic. It is still strictly more informative than the old generic message; if you'd prefer, I can use a neutral fallback wording for the non-calendar case instead.Closes #2053