fix: pad negative years to four digits in ISO 8601 output#12
Conversation
Add pad4_year so the year field is sign plus four-digit absolute value (e.g. -0500, -0001). Use it in DateTime::format and Show for Date. Closes #5. Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR fixes ISO 8601-style formatting for negative years by ensuring the year field is zero-padded to four digits after applying the sign, and adds regression tests to cover BC/negative-year formatting.
Changes:
- Introduce
pad4_year(year)to format signed years as-+ zero-padded absolute value. - Use
pad4_yearinDateTime::formatandShowforDate. - Add blackbox tests for
-500and-1for bothformat()andShow, and ignore.cursor/in.gitignore.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tempo.mbt |
Adds pad4_year and routes Date/DateTime formatting through it to correct negative-year padding. |
src/tempo_test.mbt |
Adds regression tests asserting correct formatting/Show output for negative years. |
.gitignore |
Ignores .cursor/ editor metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Year field for ISO 8601 / RFC 3339: sign plus four-digit absolute value. | ||
|
|
||
| ///| | ||
| fn pad4_year(year : Int) -> String { |
There was a problem hiding this comment.
pad4_year negates year to compute the absolute value. If year can ever be the minimum representable Int, -year will overflow/wrap and produce an incorrect string (potentially with a doubled -). Consider using an overflow-safe absolute-value conversion (or explicitly rejecting/guarding that input range for Date::new).
| fn pad4_year(year : Int) -> String { | |
| fn pad4_year(year : Int) -> String { | |
| if year == Int::MIN { | |
| panic("pad4_year: year is Int::MIN, which is not supported"); | |
| } |
| // Year field for ISO 8601 / RFC 3339: sign plus four-digit absolute value. | ||
|
|
||
| ///| | ||
| fn pad4_year(year : Int) -> String { | ||
| if year < 0 { | ||
| "-" + pad4(-year) | ||
| } else { | ||
| pad4(year) | ||
| } |
There was a problem hiding this comment.
The comment says “ISO 8601 / RFC 3339: sign plus four-digit absolute value”, but (1) RFC 3339 does not allow signed/expanded years, and (2) pad4 doesn’t enforce exactly 4 digits for years ≥ 10000. Please update the comment to reflect the actual behavior/spec you intend to support (e.g., ISO 8601 expanded year with at least 4 digits), or adjust the implementation to match the documented constraint.
|
Choir: comment-only review received — routed to |
Reject Int::min_value in pad4_year with abort (negation overflow). Document RFC 3339 vs ISO 8601 expanded-year behavior and pad4 rules for large years. Import moonbitlang/core/int for @int.min_value. Made-with: Cursor
|
Choir: fixes pushed by |
Summary
pad4treated negative years as a single signed integer, so year-500formatted as-500instead of the ISO 8601-style-0500(sign plus four-digit absolute value).Changes
pad4_year(year)that prefixes-and appliespad4to the absolute value whenyear < 0; otherwise delegates topad4.DateTime::formatandShowforDate.ShowforDateTimealready usesformat(), so it picks up the fix automatically.-500and-1(format andShow)..cursor/so local editor metadata does not block PR tooling.Closes #5.