Skip to content

fix(naive_time): enforce exact width when parsing %H%M%S #1710

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kumarUjjawal
Copy link

Summary

Fixes #1697: NaiveTime::parse_from_str was accepting inputs shorter than the expected length for format strings like "%H%M%S". For example, "01023" was being parsed as 01:02:03, which is incorrect.

According to the documentation:

%S represents "Second number (00–60), zero-padded to 2 digits."

Bug

However, NaiveTime::parse_from_str("01023", "%H%M%S") silently succeeds, returning 01:02:03. I expected it to fail due to input being too short for %H%M%S (which should be 6 digits). This can lead to subtle bugs if users pass malformed data.

Examples

Input Format Result Expected
"010233" %H%M%S 01:02:33 OK
"0102334" %H%M%S ❌ Error OK
"01023" %H%M%S 01:02:03 ❌ Should error

Fix

I modified parse_internal to enforce exact width parsing for numeric fields like %H, %M, %S, etc., rejecting inputs that are too short or too long.

I also updated/added test cases to reflect this.

Open Questions

  • Would you prefer strict parsing be the default?
  • Or should I gate this behind a feature flag (e.g. strict-numeric) to avoid breaking changes?

Why This Matters

The current behavior contradicts the documentation and can lead to unexpected results when parsing time strings. Enforcing exact width aligns with user expectations and format specifiers.

Please let me know if this direction makes sense. I’d be happy to:

  • Adjust tests or behavior,
  • Document the strict parsing behavior more clearly, or
  • Provide a feature flag if you'd prefer this not to be the default.

Closes #1697

@djc
Copy link
Member

djc commented Jun 25, 2025

I think this is too incompatible for the 0.4.x release (as can be witnessed from all the test failures). We could take this on in the 0.5.x branch (if it isn't there already, I forget).

@kumarUjjawal
Copy link
Author

That makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing "%H%M%S" has either a bug or incorrect description
2 participants