Flexible offset formatting [1/3] by pitdicker · Pull Request #1082 · chronotope/chrono

pitdicker · 2023-05-24T18:56:25Z

There are a lot of issues with our current code to parse time zone offsets. For example %z, %:z, %::z and %:::z are all treated identical in the parser. In some cases Z is accepted, and there is a hack to accept UTC. There is no way to parse seconds.

Formatting is in a better state, but it would be nice if it could be more flexible. For example to be able to print seconds optionally, support padding, or print Z.

This is part 1 of a series of PRs to improve offset formatting and parsing. (1) adds a flexible formatter, (2) adds a flexible parser and fixes current parsing bugs, and (3) exposes the functionality in the format string.

With this PR it is now possible to specify the format of an offset:

Is the precision in hours, minutes, seconds, or are minutes and/or seconds optional?
Are the values seperated by a colon, nothing, or (when parsing) are both acceptable?
Is Z used for +00:00?
Should the hour value be zero-padded, space-padded (before the sign), or not be padded?

jtmoon79

LGTM

src/format/mod.rs

djc · 2023-06-29T12:12:45Z

src/format/mod.rs

-                        } else {
-                            locale.am_pm[0]
-                        });
+            let ret = match *spec {


Why is this in a separate commit?

To make it easier for you to see what is changed, and what is caused by rustfmt.

So this is old code that was previously not formatted? What changed that rustfmt is now formatting it?

I did some experiments at the time, but couldn't pin it down. In #1126 (comment) I run into the same issue.

Want to file a rustfmt issue?

src/format/mod.rs

djc

Okay, please fix up these final nits (unless you disagree) and feel free to merge.

djc · 2023-06-30T19:59:17Z

src/format/formatting.rs

 #[cfg(feature = "unstable-locales")]
 use super::locales;
 #[cfg(any(feature = "alloc", feature = "std"))]
+use super::{Colons, OffsetFormat, OffsetPrecision};


We can merge these imports, right?

djc · 2023-06-30T19:59:43Z

src/format/formatting.rs

-                        off.map(|&(_, off)| write_local_minus_utc(result, off, true, Colons::None))
-                    }
+                    TimezoneOffset | TimezoneOffsetZ => off.map(|&(_, off)| {
+                        let allow_zulu = *spec == TimezoneOffsetZ;


I'd prefer inlining this value in the OffsetFormat::new() call.

djc · 2023-06-30T19:59:54Z

src/format/formatting.rs

+                        .format(result, off)
+                    }),
+                    TimezoneOffsetColon | TimezoneOffsetColonZ => off.map(|&(_, off)| {
+                        let allow_zulu = *spec == TimezoneOffsetColonZ;


djc · 2023-06-30T20:01:45Z

src/format/mod.rs

+
+impl OffsetFormat {
+    /// Create a new `OffsetFormat`.
+    pub const fn new(


Given that the new() calls will still be spread over the same number of lines in practice, I feel like this method isn't really buying us anything, whereas having the field names in the initializer would make the code a little easier to understand (particularly for the allow_zulu field).

djc · 2023-06-30T20:02:04Z

src/format/mod.rs

+pub struct OffsetFormat {
+    /// See `OffsetPrecision`.
+    pub precision: OffsetPrecision,
+    /// Seperator between hours, minutes and seconds.


One more instance of sepe

Thank you for being thorough, I thought I had them all.

pitdicker force-pushed the offset_formatting_1 branch 3 times, most recently from 10e49a9 to df6a2d0 Compare May 24, 2023 19:28

This was referenced May 24, 2023

Flexible offset parsing [2/3] #1083

Draft

Do not use Offset's Debug impl when serializing DateTime #1035

Merged

jtmoon79 approved these changes May 28, 2023

View reviewed changes

djc added the waiting-on-review label May 28, 2023

pitdicker mentioned this pull request Jun 5, 2023

Make formatting available without alloc #1126

Closed

pitdicker force-pushed the offset_formatting_1 branch from df6a2d0 to 492c83c Compare June 8, 2023 18:16

djc reviewed Jun 29, 2023

View reviewed changes

pitdicker force-pushed the offset_formatting_1 branch 3 times, most recently from e687926 to d27793b Compare June 29, 2023 12:46

djc reviewed Jun 30, 2023

View reviewed changes

src/format/mod.rs Outdated Show resolved Hide resolved

src/format/mod.rs Outdated Show resolved Hide resolved

src/format/mod.rs Outdated Show resolved Hide resolved

pitdicker force-pushed the offset_formatting_1 branch 7 times, most recently from f84e948 to fd0f41b Compare June 30, 2023 16:57

djc approved these changes Jun 30, 2023

View reviewed changes

pitdicker force-pushed the offset_formatting_1 branch from fd0f41b to 2cce338 Compare June 30, 2023 20:47

pitdicker added 3 commits June 30, 2023 22:50

New offset formatter OffsetFormat

3dda28c

Test offset formatter

29019fb

Rustfmt

edf413d

pitdicker force-pushed the offset_formatting_1 branch from 2cce338 to edf413d Compare June 30, 2023 20:51

pitdicker merged commit e0e4303 into chronotope:0.4.x Jun 30, 2023

pitdicker deleted the offset_formatting_1 branch June 30, 2023 21:03

pitdicker mentioned this pull request Jul 31, 2023

Parse Zulu suffix with %z #1210

Closed

pitdicker mentioned this pull request Aug 28, 2023

Explore using icu4 #1227

Closed

pitdicker mentioned this pull request Feb 7, 2024

Recognize our review capacity is limited for non-trivial work #1417

Open

Conversation

pitdicker commented May 24, 2023

Uh oh!

jtmoon79 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants