Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support #205

dekellum · 2018-01-12T20:04:53Z

These additions allow convenient control of RFC 3339 formatted output:

Number of subsecond digits to display
Whether to use the 'Z' variant, instead of "+00:00" for TZ offset
0, UTC.

...while remaining faithful to the RFC 3339. The implementation uses the existing formatting Item mechanism.

The 'Z' variant was originally requested in #157 where it was suggested a PR was welcome. Control of subsecond digits was requested in #178 and @quodlibetor gave me a starting point for using the formatting Item mechanism.

Tests and rustdoc are included. Please consider merging this or let me know if it has any deficits, thanks!

quodlibetor · 2018-01-12T20:55:08Z

Awesome, thanks! I'll review this over the weekend, I want to do a survey of our existing API to make sure that we are working towards something consistent.

dekellum · 2018-01-12T21:28:57Z

Thanks. While Travis CI is delayed waiting for osx builds, there are two failures so far:

I used a static for the PREFIX rfc3339 Item array, but this has only been supported since rustc 1.17 (Stabilize static_recursion rust-lang/rust#40027). Shall I change this to const? Alternately could make 1.17 the minimum version, but then there are probably other cases to make static or other bigger changes associated with that.
The build with clippy is failing on existing code, not these changes, but its somewhat related to the above, e.g:

error: Constants have by default a `'static` lifetime
   --> src/datetime.rs:463:23
         const ITEMS: &'static [Item<'static>] = &[
                       ^^^^^^^ help: consider removing `'static`

I assume that lint is new since the last successful build a few months ago. I'm willing to try and fix that as well, depending on how to solve (1), but that might be better in its own PR?

quodlibetor · 2018-01-12T21:58:18Z

We want to preserve compat back to rust 1.13, so we should disable that clippy lint. Feel free to do it in this PR but in a separate commit, thanks! By the same logic, please change it to const.

I've enabled weekly builds so that we'll get alerted if clippy changes cause things to fail and future contributors won't have to deal with this.

github: cc: chronotope#205

These additions allow convenient control of RFC 3339 formatted output: * Number of subsecond digits to display * Whether to use the 'Z' variant, instead of "+00:00" for TZ offset 0, UTC. ...while remaining faithful to the RFC 3339. The implementation uses the existing formatting Item mechanism. github: cc: chronotope#157 chronotope#178

Was using static but that's only supported as of rustc 1.17 (rust these older versions. Also continue using the copious explicit 'static lifetimes for the same compatibility, despite the clippy lint.

so it builds with rustc 1.13

dekellum · 2018-01-18T19:14:26Z

Thanks for merging #206. I've rebased this branch hoping to eventually (looking at you MacOS) see passing CI for the PR.

dekellum · 2018-01-23T16:55:23Z

@quodlibetor: Tests are passing. Did you have a chance to survey and consider this regarding API consistency?

In case it helps: I'm not attached to the introduced -p(z) suffix naming. The 'z' was based on comments in #157. I'm not sure if there is a better Rust style available here vs this sort of libc naming style.

Also this currently uses the Fixed enum for the sub-second formatting parameter. That's arguably consistent, but at the expense of a potential panic to enforce RFC 3339 constraints at runtime. An alternative would be to introduce a new enum for just the sub-second formats supported.

Please let me know your preferences or other concerns.

Perhaps I didn't make a strong enough case for this addition in the first place: I believe the 'Z' and fixed sub-second format variants of RFC-3339 are too commonly needed to not include in this crate, particularly when considering the amount of code required to correctly implement it externally.

quodlibetor · 2018-01-24T16:52:28Z

Two things:

I definitely want to get this merged, there's little debate in my mind that we need something like this
My concern was with the Option<Fixed> enum, I don't believe that there are any other examples of us taking such a complex parameter to something
Sorry, my day job is day, night and weekend job for the last month or so which is why this is languishing

I think that taking a new enum (especially one that statically prevents errors) will be more ergonomic, despite the extra import.

My larger concern is with formatting in general -- the fact that this is complex to handle this in an external crate is a bug which should be fixed, but I think that we basically just need to create a new formatting API to manage it.

If you want to help me out, the survey that I wanted to do was mostly just to look at our various other formatting APIs and see:

Do any methods take enums? If so, how complex are they?
Do the Rust API Guidelines have anything to say about API's like this?
Should we be documenting these more complex formatting methods in terms of the underlying format method that takes a slice of Format::Item variants so that external users are less dependent on us to handle this for them?

dekellum · 2018-01-25T23:13:27Z

In the latest I've introduced a SecondsFormat enum. This simplifies the function enough that I also went with a single method, named to_rfc3339_opts (which I think is a more rust-style name) and use a single bool for use of the Z-variant. The new enum is included in datetime.rs and pub export at chrono:: top level. I didn't put this in chrono::format because its role is now more as a façade over the format interfaces.

In my evolving understanding of the formatting API, I share your larger concern. Error handling appears problematic with parsing formats from strings. Working with the format Items directly is complicated by enum/variant name collisions and the need for cloned value iteration? It would be nice if there was some easy way to create a static or constant Format(-ter) , for this application from a create_rfc_3339_format(args) kind of function, and then simple use that repeatedly over many DateTime values.

But this PR makes chrono more usable now and in the interim. When there is a a better format module, DateTime::to_rfc_3339 and to_rfc_3339_opt can be reimplemented in terms of that, and possibly also deprecated.

quodlibetor

The doc test improvements are important, but this looks great and I'd be comfortable merging it.

Some time in the future I think this enum could actually be extended to enhance the use case from #165 , too.

quodlibetor · 2018-01-26T14:02:05Z

src/datetime.rs

@@ -1059,6 +1133,28 @@ mod tests {
                   Ok(EDT.ymd(2015, 2, 18).and_hms_micro(23, 59, 59, 1_234_567)));
    }

+    #[test]
+    fn test_rfc3339_opts() {


Could you add a couple of these asserts to the docs for to_rfc3339_opts? Maybe just all 4 (pst/utc,true/false) of the Millis asserts?

Yes, included in latest.

quodlibetor · 2018-01-26T14:08:11Z

src/datetime.rs

+            }
+        );
+
+        match ssitem {


this match could probably be something like:

PREFIX.iter().chain(s.iter()).flat_map(|s| s).chain([tzitem].iter()) I'm not sure off the top of my head what the correct nesting of flat_map with option would be (maybe just sticking the option in the map chain would do it?) but if you're willing to give it a shot I think it'd be cleaner.

I tried this, but I couldn't get it to compile. I'm glad we agree that the format API is hard to use though. :) I also agree the current impl looks redundant, but in my own attempts to refactor, I ran into various forms of trouble with the barrow-checker. Non-Lexical Lifetimes might make that possible in the future?

quodlibetor · 2018-01-26T15:35:05Z

src/datetime.rs

@@ -16,6 +16,30 @@ use Date;
 use format::{Item, Numeric, Pad, Fixed};
 use format::{parse, Parsed, ParseError, ParseResult, DelayedFormat, StrftimeItems};

+/// Specific formatting options for seconds
+#[derive(Clone, Copy, Debug, Eq, PartialEq)]
+pub enum SecondsFormat {


Actually: we should probably name this FractionFormat and also add a #[doc(hidden)] __Nonexhaustive variant so that we can expand this enum in the future, without it being a breaking change?

Naming: I went through some pre-PR iteration on this. Calling it SubSecFormat or FractionFormat makes one want to use something like Omit for the Secs variant and then it doesn't fit together as well. Its really intended to be one parameter that specifies the seconds format including the seconds ['.' subseconds]. Added more doc to try and clarify this.

If I add __Nonexhaustive in this, then I need to add a panic!() in to_rfc3339_opts to catch the case where it doesn't cover a new variant. The std ErrorKind example of this being used is for a case where the enum in question is being returned and consumed by external code. I don't think there is a valid use case for external code matching on SecondsFormat but I added a warning in rustdoc not to do this. I hope that is sufficient.

dekellum · 2018-01-26T20:31:22Z

Also regarding your comment on future expansion and #165 (as well as linked #171), I renamed the one variant to AutoSi and improved rustdoc to indicate what it does. Should make it easy to add additional variants in the future for other number of digits, e.g. Centis-econds or NoPad.

quodlibetor · 2018-01-30T03:16:49Z

Thank you! Sorry for the slow turnaround, but this is great!

dekellum · 2018-01-30T05:05:05Z

Thanks! Was just looking at the accumulated changes since 0.4. At some point, would be good to know if there as anything else that is particularly release gating?

dekellum added a commit to dekellum/chrono that referenced this pull request Jan 12, 2018

Add top level clippy allow const_static_lifetime

f93c8a9

github: cc: chronotope#205

dekellum mentioned this pull request Jan 12, 2018

Clippy allowances to get the build passing again #206

Merged

dekellum added 4 commits January 18, 2018 11:07

Use const for compat with rustc 1.13-16

353a7bb

Was using static but that's only supported as of rustc 1.17 (rust these older versions. Also continue using the copious explicit 'static lifetimes for the same compatibility, despite the clippy lint.

cleanup name resolution for backward compatibility

0e6d1d4

so it builds with rustc 1.13

Clippy no likey a match on bool

efb0f3b

dekellum force-pushed the rfc3339pz branch from 9ebf0ed to efb0f3b Compare January 18, 2018 19:10

Add more specific SecondsFormat and offer single to_rfc3339_opts

1a0ebe7

quodlibetor requested changes Jan 26, 2018

View reviewed changes

quodlibetor reviewed Jan 26, 2018

View reviewed changes

Rustdoc and naming improvements

e8fdbdf

quodlibetor merged commit ff962d4 into chronotope:master Jan 30, 2018

dekellum changed the title ~~Add DateTime to_rfc3339p(z) methods, tests~~ Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support Jan 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support #205

Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support #205

dekellum commented Jan 12, 2018

quodlibetor commented Jan 12, 2018

dekellum commented Jan 12, 2018

quodlibetor commented Jan 12, 2018

dekellum commented Jan 18, 2018

dekellum commented Jan 23, 2018 •

edited

Loading

quodlibetor commented Jan 24, 2018

dekellum commented Jan 25, 2018 •

edited

Loading

quodlibetor left a comment

quodlibetor Jan 26, 2018

dekellum Jan 26, 2018

quodlibetor Jan 26, 2018

dekellum Jan 26, 2018

quodlibetor Jan 26, 2018

dekellum Jan 26, 2018

dekellum commented Jan 26, 2018

quodlibetor commented Jan 30, 2018

dekellum commented Jan 30, 2018

Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support #205

Add DateTime::to_rfc3339_opts method with SecondsFormat and 'Z' support #205

Conversation

dekellum commented Jan 12, 2018

quodlibetor commented Jan 12, 2018

dekellum commented Jan 12, 2018

quodlibetor commented Jan 12, 2018

dekellum commented Jan 18, 2018

dekellum commented Jan 23, 2018 • edited Loading

quodlibetor commented Jan 24, 2018

dekellum commented Jan 25, 2018 • edited Loading

quodlibetor left a comment

Choose a reason for hiding this comment

quodlibetor Jan 26, 2018

Choose a reason for hiding this comment

dekellum Jan 26, 2018

Choose a reason for hiding this comment

quodlibetor Jan 26, 2018

Choose a reason for hiding this comment

dekellum Jan 26, 2018

Choose a reason for hiding this comment

quodlibetor Jan 26, 2018

Choose a reason for hiding this comment

dekellum Jan 26, 2018

Choose a reason for hiding this comment

dekellum commented Jan 26, 2018

quodlibetor commented Jan 30, 2018

dekellum commented Jan 30, 2018

dekellum commented Jan 23, 2018 •

edited

Loading

dekellum commented Jan 25, 2018 •

edited

Loading