Skip to content
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

Time v0.3 tracking issue #248

Closed
jhpratt opened this issue Apr 20, 2020 · 55 comments
Closed

Time v0.3 tracking issue #248

jhpratt opened this issue Apr 20, 2020 · 55 comments
Assignees
Labels
A-core Area: anything not otherwise covered A-formatting Area: formatting A-macros Area: macros A-parsing Area: parsing C-cleanup Category: cleanup of existing code C-seeking-input 📣 Category: community input is desired C-tracking-issue Category: tracking issue for a feature/release
Milestone

Comments

@jhpratt
Copy link
Member

jhpratt commented Apr 20, 2020

Time v0.2 was an outstanding learning experience, and by all accounts is a massive improvement over v0.1. However, a number of ill-informed decisions were made which led to some inconsistencies and many deprecated APIs.

All deprecated methods and structs have ready-to-use alternatives, which will be detailed before a release. As such, I would like to remove all deprecated APIs. This currently includes the following.

Deprecated APIs
  • v0.1 APIs, currently behind a default feature flag
    • PreciseTime
    • SteadyTime
    • precise_time_ns
    • precise_time_s
    • Instant::to
    • Duration::num_weeks
    • Duration::num_days
    • Duration::num_hours
    • Duration::num_minutes
    • Duration::num_seconds
    • Duration::num_milliseconds
    • Duration::num_microseconds
    • Duration::num_nanoseconds
    • Duration::span
    • Duration::from_std
    • Duration::to_std
  • Panicking APIs, currently behind a non-default feature flag
    • Date::from_ymd
    • Date::from_yo
    • Date::from_iso_ywd
    • Date::with_hms
    • Date::with_hms_milli
    • Date::with_hms_micro
    • Date::with_hms_nano
    • Time::from_hms
    • Time::from_hms_milli
    • Time::from_hms_micro
    • Time::from_hms_nano
  • APIs that assume an offset of UTC, currently enabled unconditionally
    • Date::today
    • Time::now
    • PrimitiveDateTime::now
    • PrimitiveDateTime::unix_epoch
    • PrimitiveDateTime::from_unix_timestamp
    • PrimitiveDateTime::timestamp
    • OffsetDateTime::now
  • Other APIs that were deprecated during the course of v0.2, currently enabled unconditionally
    • Sign
    • Duration::sign
    • PrimitiveDateTime::using_offset

Goals for v0.3:

  • Remove all deprecated APIs
  • Revamped parsing/formatting (Revamped parsing/formatting #236)
  • un-constify is_leap_year and days_in_year for performance. On Rust ≥ 1.46, cfg_if_match and cfg_loop are stable, which allows re-enabling this without runtime performance loss. As a result, the only "loss" is un-constifying methods on older compilers.
  • Place macros behind a feature flag.
  • Change a number of methods to return Result, including all UtcOffset constructors.
  • Rename try_from_* to just from_*.
  • #![no_alloc] support
  • Rename OffsetDateTime::timestamp and OffsetDateTime::timestamp_nanos to OffsetDateTime::unix_timestamp and OffsetDateTime::unix_timestamp_nanos respectively.
  • Stabilize serde representations

Anything stricken on the above list is completed. Anything that was previously on the list but was removed was either done in 0.2 (if it's not a breaking change) or abandoned.

@jhpratt jhpratt added C-seeking-input 📣 Category: community input is desired C-tracking-issue Category: tracking issue for a feature/release labels Apr 20, 2020
@jhpratt jhpratt self-assigned this Apr 20, 2020
@jhpratt
Copy link
Member Author

jhpratt commented May 4, 2020

As a result of packing the Date struct into a single field, a number of structs have been reduced in size.

struct bytes before bytes now reduction
Time 8 8 0%
Date 8 4 50%
UtcOffset 4 4 0%
PrimitiveDateTime 16 12 25%
OffsetDateTime 20 16 20%

A simple benchmark (not published) also shows a ~30% improvement in performance!

@jhpratt jhpratt added A-core Area: anything not otherwise covered C-cleanup Category: cleanup of existing code labels May 16, 2020
@SergioBenitez
Copy link

As a suggestion, I think it would be useful to have a release candidate for 0.3, when you feel it's ready, to work out any remaining issues in dependent crates. There are a few API nags in 0.2 that I'd love to see resolved in 0.3.

@jhpratt
Copy link
Member Author

jhpratt commented May 22, 2020

@SergioBenitez Definitely planning on a release candidate. Main thing I want to get done is the revamped formatting, as that's a major change.

Are there any APIs in particular you'd prefer changed that aren't addressed by the list above?

@jhpratt
Copy link
Member Author

jhpratt commented May 25, 2020

An update on no-alloc support:

Once revamped formatting/parsing is complete, I'll likely release an alpha for initial review.

While I'm designing the formatting/parsing with no-alloc support in mind, support for dynamic formatting strings seems unlikely, at least at first. The ability to have modifiers for the various specifiers would be quite inefficient (quadratic versus linear for checking in alloc environments). However, this doesn't preclude the possibility of statically known formats, namely those in the time::format mod or something procedurally generated via a macro).

I will investigate the possibility for no-alloc support for other operations. Given that error handling for constructors currently required Vec to be present (to store the year and month for whether a day is valid, for example), it seems unlikely no-alloc support will land in the initial 0.3.0 release.

I could use the arrayvec and/or heapless crates, but dependencies can't (currently) be disabled by enabling a feature flag (name alloc).

@SergioBenitez
Copy link

@jhpratt I'm not sure what's changed in 0.3 thus far, so these may be outdated, but, here are some notes:

  1. Macros should be declarative, not procedural, where possible. At a first glance, this seems totally possible. This would remove the heavy syn, quote, proc-macro-hack, etc. dependencies.
  2. Creating a constant date is very verbose. This is similar to OffsetDateTime::replace_time? #256. Why do I need to call assume_utc() and to_offset()? As far as I can see, there's no way to create an OffsetDateTime in one line. I should be able to do datetime!(2020-06-05 12:34pm GMT) or what-have-you.
  3. Docs do a poor job of explaining the differences between PrimitiveDateTime and OffsetDateTime and how to move between the types. Why isn't there simply Date, Time, and DateTime, the latter of which could or couldn't have an offset? If the type difference is important, I'd still call it DateTime instead of PrimitiveDateTime; then OffsetDateTime, makes a bit more sense and establishes a more obvious hierarchy.
  4. In general, the docs should describe common operations and why the types that exist do, and why we should care. Handling time correctly is hard which may lead to more complexity in a time handling library. When it does, you should explain this so we can appreciate it.
  5. I'd encourage the use of modules. You currently have multiple structs whose names end with Error; these should all be in an error module without the Error suffix. Similarly, there are several free-standing functions that don't really seem to fit with the rest of the crate; should these be in a util module? In general, the catalog of top-level types and modules should already give me an idea of how the library is to be used.
  6. I like this way of presenting features. It's easy to see which features exist.

@jhpratt
Copy link
Member Author

jhpratt commented Jun 5, 2020

  1. I would love to be able to use macro_rules!. There are two things that can happen that would allow this to happen. Either would suffice.

    • Stabilization of const_if_match and const_panic. I'd have to check, but const_loop might also be necessary for some use cases. This would allow the macro to expand to Date::try_from_ymd($year, $month, $day).unwrap(), which would not compile if it was in a const context. I'm not sure on the semantics if it weren't in a const context — I presume it would continue to compile and panic at runtime, something that's undesirable.
      • const_if_match and const_loop are both well on their way to stabilization. const_panic is getting there, but it definitely won't land at the same time.
    • Re-interpretation of tokens inside of macro_rules! matchers. If this were possible, I'd be able to have one macro that verified a number is between 0 and 59, for example. A series of these could then be used to verify the full value.
      • I'm not aware of any plans for this to be allowed.

    Essentially, the reason it's a proc macro now is because it allows you to statically verify the validity of a value, allowing it to be used in const contexts.

  2. When you call .assume_utc().to_offset(…), you are performing two logical operations. Once the aforementioned const features land on stable, .assume_offset() will be usable in const contexts. Going from a PrimitiveDateTime to an OffsetDateTime should require explicitly assuming an offset, even if it is UTC.

    I was planning on looking into a way to directly construct an OffsetDateTime. I'm not sure what form this would take, though. Macros would suffice for static values. I can look into whether a macro would be feasible, though I think it could work.

  3. Funny you suggest merging them. I have actually looked into using typestate, but I believe the issue was the lack of ability to have const fn in traits. I can re-investigate this, though, as I may be misremembering things.

  4. Any particular areas that could use improvement?

  5. Modules is one thing I wanted to look into, but hadn't yet. Having a util module makes sense, at a minimum.

  6. Can do! I knew the way I had it was a bit kludgey, but didn't have a better way.

@jhpratt jhpratt pinned this issue Jun 6, 2020
@SergioBenitez
Copy link

I would love to be able to use macro_rules!. There are two things that can happen that would allow this to happen. Either would suffice.

I suppose I'm less concerned about the fact that it's a proc-macro and more concerned about the heavy-weight dependencies being pulled in. The macros are fairly straight-forward, so you should be able to accomplish the same thing without pulling in any dependencies whatsoever.

When you call .assume_utc().to_offset(…), you are performing two logical operations. Once the aforementioned const features land on stable, .assume_offset() will be usable in const contexts. Going from a PrimitiveDateTime to an OffsetDateTime should require explicitly assuming an offset, even if it is UTC.

I suppose I'm confused about calling assume_utc() on a PrimitiveDateTime. The "assume" part is odd; why isn't there simply a fn as_offset(..) -> OffsetDateTime? Presumably the entire point of PrimitiveDateTime is that there is no offset. Still, I'd really like a datetime!() macro where an offset can be specified to construct an OffsetDateTime directly.

Funny you suggest merging them. I have actually looked into using typestate, but I believe the issue was the lack of ability to have const fn in traits. I can re-investigate this, though, as I may be misremembering things.

I believe you're suggesting something like: DateTime<Raw> and DateTime<Offset>. This is easy enough to accomplish, but there's a trade-off here in terms of usability. I'm suggesting not disambiguating between a DateTime with and without an offset, or perhaps said differently, provide a default offset (of +/- 0) for a DateTime without an offset, and then allow it to be changed without it affecting the type. Thus, there would be a single DateTime, not two as it stands now. As mentioned before, this is only viable if the ability to disambiguate at the type-level isn't particularly important.

  1. Any particular areas that could use improvement?

I've noted a few in my comment already. Each different date/time type needs to detail 1) why it exists, 2) how to create it, and 3) how it relates to the other types. There should also be a high-level overview of the library on common ways it's intended to be used.

@jhpratt
Copy link
Member Author

jhpratt commented Jun 12, 2020

I suppose I'm less concerned about the fact that it's a proc-macro and more concerned about the heavy-weight dependencies being pulled in. The macros are fairly straight-forward, so you should be able to accomplish the same thing without pulling in any dependencies whatsoever.

The original reason for pulling in the dependencies was proc-macro-hack. I can likely take a look at how that actually works and write the code myself (until the crate's MSRV allows for proc_macro_hygiene). The parsing isn't terribly difficult by itself (that was my original goal), though I do wish Rust provided something more than "literal" for parsing. Casting to a string and re-parsing just seems…stupid.

I suppose I'm confused about calling assume_utc() on a PrimitiveDateTime. The "assume" part is odd; why isn't there simply a fn as_offset(..) -> OffsetDateTime? Presumably the entire point of PrimitiveDateTime is that there is no offset. Still, I'd really like a datetime!() macro where an offset can be specified to construct an OffsetDateTime directly.

Something like PrimitiveDateTime::assume_offset? It's not const due to compiler limitations.

Also, turns out it was trivial to create a datetime! macro to construct a PrimitiveDateTime. It's now possible to do datetime!(2020-01-01 0:00).assume_offset(UtcOffset::UTC), though that does require the end user to add in the offset themselves. Expanding it to generate an OffsetDateTime would require additional code duplication in the macros crate. I'm not necessarily opposed to that, though it honestly isn't a priority.

I believe you're suggesting something like: DateTime<Raw> and DateTime<Offset>. This is easy enough to accomplish, but there's a trade-off here in terms of usability. I'm suggesting not disambiguating between a DateTime with and without an offset, or perhaps said differently, provide a default offset (of +/- 0) for a DateTime without an offset, and then allow it to be changed without it affecting the type. Thus, there would be a single DateTime, not two as it stands now. As mentioned before, this is only viable if the ability to disambiguate at the type-level isn't particularly important.

Yes, that's the sort of thing I was referring to. One major reason to keep the types separate is that a PrimitiveDateTime doesn't require the storage of an offset. I don't care for the idea of assuming a given offset (even UTC) when none is specified, as that can lead to surprising behavior, particularly while parsing, though I could likely be convinced.

With regard to renaming PrimitiveDateTime, @KodrAus expressed concern over this naming prior to the 0.2.0 release.

I've noted a few in my comment already. Each different date/time type needs to detail 1) why it exists, 2) how to create it, and 3) how it relates to the other types. There should also be a high-level overview of the library on common ways it's intended to be used.

👍

@Plecra
Copy link

Plecra commented Jul 22, 2020

In case it helps, here is a minimal implementation of proc-macro-hack using macro_rules. It doesn't implement support_nested or fake_call_site, neither of which should be needed here.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 22, 2020

Thanks @Plecra. I didn't realize it at the time, but proc macro hack actually no longer relies on syn and quote. So relying on that for a sufficient time period (such that my MSRV guarantees are upheld) is a non-issue. The time to compile it by itself is negligible enough that I'm fine keeping it.

@jhpratt jhpratt added A-formatting Area: formatting A-macros Area: macros A-parsing Area: parsing labels Sep 23, 2020
@yorickpeterse
Copy link

@jhpratt It seems that time-macros does indirectly depend on syn, through the time-macros-impl crate. Are there any plans on removing that in the nearby future? The syn crate isn't always the fastest to compile, and earlier today it even segfaulted (that may have just been bad luck).

@jhpratt
Copy link
Member Author

jhpratt commented Sep 30, 2020

@yorickpeterse It's already been removed on the v0.3 branch.

@yorickpeterse
Copy link

@jhpratt Ah thanks! In that case I'll keep an eye on this issue.

@hummingly
Copy link

I don't know if that is the correct issue for this but it seems you're looking for feedback on the API, so I will just dump my thoughts here. The reason I am checking out this crate is that I want update an old crate of mine (I am talking about full face lift level update) and add the time crate as optional dependency to use its types or at least conversions. Overall I am not in a rush because there is so much work 😅

Naming Inconsistencies

  • The constructor methods essentially convert one ISO date to a Date. That's why Date::from_iso_ywd and Date::yo feel a bit random chosen because there are actual terms defined in the standard. The terms ordinal date or week date seems overall easier to find and search for. Otherwise rustdoc has now this alias feature to make it possible to search for those terms.

    • Date::from_ymd stays the same but I would note in the comment that the ISO calendar date representation is meant.
    • Date::from_iso_ywd -> Date::from_week_date (part of ISO standard anyway so redundant)
    • Date::from_yo -> Date::from_ordinal_date
    • Date::from_julian_day -> Date::from_julian_date (keeping consistenst with the other constructors)
  • The Date type also has methods to convert back into the desired ISO representation. Date::julian_day and Date::iso_year_week are named as getters even though they are not as cheap to access. The as-prefix is also only appropriate for Date::as_yo as it pretty much the underlying data structure and the others are more expensive to compute. into did not make sense because Date is Copy and not being converted to a completely different data type. They are still dates, so I chose the to-prefix.

    • Date::as_ymd -> Date::to_ymd
    • Date::iso_year_week -> Date::to_week_date
    • Date::as_yo -> Date::as_ordinal_date (to-prefix might still be the better choice)
    • Date::julian_day -> Date::to_julian_date
  • I prefer the and-prefix for constructing a new type from an instance because the with-prefix is usually used for constructors.

    • Date::with_hms_* -> Date::and_hms_*
    • Date::with_time -> Date::and_time

API Inconsistencies

  • Date::iso_year_week should return the week day too because every other method that changes the representation returns the same values that is used for the conversion constructor parameters. This way input and output would be the same and it would be easier to learn the API.
  • Why is Date::from_yo_unchecked public and hidden in the docs? Can't it be just pub(crate)?

OffsetDateTime

At first I wanted to suggest to rename it to DateTime but the situation is definitely more complex. I thought at first that the tyoes in this crate are not time zone aware but then OffsetDateTime::now_local wouldn't be fallable. The problem is that the definition of date times as defined in ISO 8601 do not require time zone designators.

date-time = date "T" time [ ("Z" / "UTC") | utc-offset ]

There in fact two types of date times that are still date times at the end of the day. Floating date times are ambigious or relative to the observer. If I said I always want to go jogging at 17 o' clock, it wouldn't matter whether I am in Germany or China. Though for server logs one would prefer to have them in UTC time or at least local time because servers are not necessarily in the same time zone as the developer or customer.
The alternative would be an enum but I'm not sure how ergonomic that is and the size would be bigger.

Overall the current API for date times is fine but it lacks documentation. FloatingDateTime wouldn't be so bad but I'm unsure how many people would actually understand it.

Misc
Maybe add to the top level documentation that this crate is for working with gregorian dates and that fractal seconds aren't supported.

I hope that this long comment is useful for development!

@jhpratt
Copy link
Member Author

jhpratt commented Nov 27, 2020

I don't know if that is the correct issue for this but it seems you're looking for feedback on the API, so I will just dump my thoughts here.

This issue works!

The reason I am checking out this crate is that I want update an old crate of mine (I am talking about full face lift level update) and add the time crate as optional dependency to use its types or at least conversions. Overall I am not in a rush because there is so much work 😅

I sure hope you're not in a rush, because I've still no solid timeline for time 0.3 😛 My best guess is late this year or early next.

Naming Inconsistencies

  • The constructor methods essentially convert one ISO date to a Date. That's why Date::from_iso_ywd and Date::yo feel a bit random chosen because there are actual terms defined in the standard. The terms ordinal date or week date seems overall easier to find and search for. Otherwise rustdoc has now this alias feature to make it possible to search for those terms.

With regard to ywd, I chose to explicitly put in "iso" because there are a number of ways to number weeks. In North America, it's common (relatively speaking) to number weeks Sunday-to-Saturday. Outside North America, it's typically the same but Monday to Sunday. In both of these situations, the year is always the same as the calendar year. ISO, on the other hand, says that a week is Monday-to-Sunday but counts the weeks in a slightly different way. As such, I felt it best to be explicit about the counting method is expected to be passed as a parameter.

  • Date::from_ymd stays the same but I would note in the comment that the ISO calendar date representation is meant.

If we're changing the others, we may as well change this one to Date::from_calendar_date as well.

  • Date::from_iso_ywd -> Date::from_week_date (part of ISO standard anyway so redundant)

As noted above. Date::from_iso_week_date may be reasonable.

  • Date::from_yo -> Date::from_ordinal_date

👍

  • Date::from_julian_day -> Date::from_julian_date (keeping consistenst with the other constructors)

Technically what's being passed is actually the Julian day number. Admittedly, the difference between a JDN and Julian date is…confusing at best. A doc alias should certainly be added here.

  • The Date type also has methods to convert back into the desired ISO representation. Date::julian_day and Date::iso_year_week are named as getters even though they are not as cheap to access. The as-prefix is also only appropriate for Date::as_yo as it pretty much the underlying data structure and the others are more expensive to compute. into did not make sense because Date is Copy and not being converted to a completely different data type. They are still dates, so I chose the to-prefix.

Time for me to look over the API naming guidelines! From the table provided, to_ seems to be the correct choice.

  • Date::as_ymd -> Date::to_ymd

As with above, Date::to_calendar_date would be preferred for consistency.

  • Date::iso_year_week -> Date::to_week_date
  • Date::as_yo -> Date::as_ordinal_date (to-prefix might still be the better choice)

I'll probably wind up changing Date::as_yo to Date::to_ordinal_date for consistency, as the internal representation isn't guaranteed (it did change during v0.2).

  • Date::julian_day -> Date::to_julian_date

As with above, to_julian_day or to_julian_day_number may happen, with a doc alias as appropriate.

  • I prefer the and-prefix for constructing a new type from an instance because the with-prefix is usually used for constructors.

    • Date::with_hms_* -> Date::and_hms_*
    • Date::with_time -> Date::and_time

This one I don't quite like. with seems clearer to me, but if lots of people have found that confusing, I am willing to change it.

API Inconsistencies

  • Date::iso_year_week should return the week day too because every other method that changes the representation returns the same values that is used for the conversion constructor parameters. This way input and output would be the same and it would be easier to learn the API.

Agreed. I can change this to Date::to_iso_week_date, making the existing method internal.

  • Why is Date::from_yo_unchecked public and hidden in the docs? Can't it be just pub(crate)?

Macros! The macros provided guarantee a statically-known, const-compatible value. As all constructors have checks for the parameter validity, I needed some way to construct a Date using a public API (that is always const). It's doc-hidden because it really shouldn't be used by anyone, ever; the caller has to guarantee validity of the parameters, or odd (but deterministic) behavior could result. This is the same situation as with Time::from_hms_unchecked.

OffsetDateTime

At first I wanted to suggest to rename it to DateTime but the situation is definitely more complex. I thought at first that the tyoes in this crate are not time zone aware but then OffsetDateTime::now_local wouldn't be fallable. The problem is that the definition of date times as defined in ISO 8601 do not require time zone designators.

date-time = date "T" time [ ("Z" / "UTC") | utc-offset ]

Note that this crate makes near-zero effort to be ISO 8601 compatible.

There in fact two types of date times that are still date times at the end of the day. Floating date times are ambigious or relative to the observer. If I said I always want to go jogging at 17 o' clock, it wouldn't matter whether I am in Germany or China. Though for server logs one would prefer to have them in UTC time or at least local time because servers are not necessarily in the same time zone as the developer or customer.
The alternative would be an enum but I'm not sure how ergonomic that is and the size would be bigger.
Overall the current API for date times is fine but it lacks documentation. FloatingDateTime wouldn't be so bad but I'm unsure how many people would actually understand it.

I'm not sure if you ran across this, but PrimitiveDateTime is an OffsetDateTime without the UtcOffset. There is the ability to assume an offset on the former, which returns the latter. A PrimitiveDateTime has no way to directly get the "current" time, as doing so inherently requires an offset (which isn't present).

With regard to OffsetDateTime::now_local, obtaining the local offset requires information from the operating system. Not every operating system even has this information, and those that do may not provide a way to do so safely (see the recent security vulnerability on Unix-like operating systems for an example of this).

Misc
Maybe add to the top level documentation that this crate is for working with gregorian dates and that fractal seconds aren't supported.

Documentation definitely could use some work, no question there. Fractional seconds (which is what I presume you meant) are supported, though. The only struct that has a "seconds" component without a subsecond value is UtcOffset; this is because I'm not aware of any reasonably modern offset from UTC that is not a multiple of a full second in length.

I hope that this long comment is useful for development!

It definitely is! Thanks for making it 🙂

@hummingly
Copy link

I'm not sure if you ran across this, but PrimitiveDateTime is an OffsetDateTime without the UtcOffset. There is the ability to assume an offset on the former, which returns the latter. A PrimitiveDateTime has no way to directly get the "current" time, as doing so inherently requires an offset (which isn't present).

The PrimitiveDateTime::assume_* methods make indeed more sense now. So, OffsetDateTime are created from the "current" time and if the system does not support retrieving the local time it will return an error on the local methods. Is that correct?

For the top level documentation short lists for features, goals, non-goals, roadmap etc. could help without writing too much. Otherwise, I think the corrections you made to my comment could be a good starting point for documentation.

@RagibHasin
Copy link

RagibHasin commented Dec 3, 2020

As a breaking release, I think the time::Result type should be made pub(crate) as it clashes with std::result::Result and forces user to re-import it.

Edit: Also the re-export of error::Error should be renamed to something like TimeError.

@jhpratt
Copy link
Member Author

jhpratt commented Dec 3, 2020

@RagibHasin It's quite common for crates to have top-level exports named Result and Error. You don't have to import them; it's typically preferred to use the fully qualified path.

@RagibHasin
Copy link

@RagibHasin It's quite common for crates to have top-level exports named Result and Error. You don't have to import them; it's typically preferred to use the fully qualified path.

Oh, my bad. I actually glob imported everything from this crate. Selectively importing items would solve this.

Thanks.

@jhpratt
Copy link
Member Author

jhpratt commented Dec 4, 2020

Glob imports generally aren't a good idea either, aside from foo::prelude::*.

@RagibHasin
Copy link

Also, now as I came across serializing, it does something totally unprecedented for me. I used chrono before and am in the process of migrating to time and chrono serialized dates and times as RFC3339 format, which is user friendly.

My suggestion is to implement this for time also.

@jhpratt
Copy link
Member Author

jhpratt commented Mar 3, 2021

@Plecra I first saw that a few weeks ago. It's enabled behind the serde-human-readable flag on main! If serde is enabled without that, it will use a format intended for binary storage.

@shirshak55
Copy link

shirshak55 commented Mar 5, 2021

Today I was trying to use this crate just for getting formatted date time string as its not avail in stdlib. But unfortunately I don't see any examples, guides etc. I searched on duckduckgo but no luck.

After 10-15 minutes i found the answer but it would be nice to have guide like https://rust-lang-nursery.github.io/rust-cookbook/datetime.html which is for chrono.

println!("{}",time::PrimitiveDateTime::now().format("%Y-%m-%d-%H:%M:%S"))

@jhpratt
Copy link
Member Author

jhpratt commented Mar 5, 2021

@shirshak55 As I noted just three days ago in this issues, I still have to work on documentation for 0.3.

@ricvelozo
Copy link

Using standard ISO 8601 for human readable output by default will simplify the use with web services / JSON.

@jhpratt
Copy link
Member Author

jhpratt commented Mar 8, 2021

That's a good point, and I'll certainly consider making the change. As a bonus, I'm pretty sure it's faster, but I haven't done a direct comparison yet.

@jhpratt
Copy link
Member Author

jhpratt commented Mar 8, 2021

Quickly looked into using RFC3339 for the human readable output of serde. This won't happen for a two reasons:

  • RFC3339 is limited in range to years 0-9999, whereas the time crate supports ±9999 with no flags enabled (±999,999 with large-dates enabled)
  • RFC3339 does not support UTC offsets with precision of one second.

While most people will need neither of these, I'm not comfortable putting a restriction like that in place when it should be general purpose. ISO8601 allows for larger & smaller years if I remember correctly, but the standard is not freely available (so I can't even verify this without paying a significant amount of money). I don't recall if ISO8601 supports UTC offsets with second precision.

@ricvelozo
Copy link

I was thinking, what about create two features, serde-with-iso8601 and serde-with-large-dates, each with a default format? And for custom formats, provide modules for use with #[serde(with = "module")], regardless of the feature, so the current default doesn't matter.

@ricvelozo
Copy link

ricvelozo commented Apr 4, 2021

Example:

#[derive(Serialize)]
struct Hello {
    // use the current default
    timestamp1: OffsetDateTime,

    #[serde(with = "time::formats::iso8601")]
    timestamp2: OffsetDateTime,
}

@jhpratt
Copy link
Member Author

jhpratt commented Apr 4, 2021

As I stated, it's not possible to use RFC3339 as a serde format for the general purpose. Changing defaults by toggling a feature flag would be dangerous, as this would mean that a library and end user could each enable one on their own (having both enabled would be a compile error).

I'm not opposed to adding more well-known formats (that's why it's an entire module), but they should be able to perform a roundtrip without losing information. In theory I could have a well-known format that could only be serialized or deserialized, but I'm not sure why that would be the case. I split the trait into two already, so the technical means to do so already exists.

Serde-specific formats would go under the time::serde module, which is where you'll find an alternative format that uses the Unix timestamp for OffsetDateTime. Given that all that's needed is a single line for the end user, there isn't much reason to place this behind a feature flag that would roughly double CI time.

@jhpratt
Copy link
Member Author

jhpratt commented Apr 22, 2021

Time 0.3.0-alpha.0 is out! The changelog is mostly accurate, though there's certainly some things to be updated. For documentation of the new format description, check out this page. I intend on filling out more of the book and updating some inline documentation before a final 0.3 release.

@SergioBenitez
Copy link

Looks like all of the items have been checked off. How do you feel about a proper stable release?

@jhpratt
Copy link
Member Author

jhpratt commented Jul 28, 2021

It's mostly documentation to be honest. I just started working on the book again yesterday, but I've got quite a bit else going on. I should figure out how to best present the format description info in the inline documentation. Other than that there's a couple places where I think documentation is inaccurate with recent changes. Once these happen it should be all set 🤞🏽

@jhpratt
Copy link
Member Author

jhpratt commented Jul 28, 2021

^^ a migration guide also needs to be made

@SergioBenitez
Copy link

SergioBenitez commented Jul 28, 2021

If the functionality is complete, I would encourage you to publish even without the book or migration guide. I suspect that the utility of these extra pieces of documentation are not of significant enough value, given the nature of the library, to warrant delaying the release. And, in my experience, these pieces tend to take an inordinate amount of time.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 28, 2021

That's reasonable. I'll take a bit of time tonight to ensure none of the existing documentation is outright wrong and check that the changelog is up-to-date. Neither of those should take too long. If it goes well I'll cut a release tomorrow.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 29, 2021

Pending the outcome of the full powerset on a number of targets (https://github.com/time-rs/time/actions/runs/1077701313), the current master branch will become v0.3.0. It normally takes a bit over an hour to run, so I'll get to it when I'm on my laptop next.

@SergioBenitez
Copy link

SergioBenitez commented Jul 29, 2021

Looks like the CI is green!

One thing I noticed in giving this a go is that const_fn and standback are the only direct dependencies by default. This is fantastic, but I think it would be even better if there were no dependencies by default. After all, time should be a fairly lightweight dependency itself. Both of these dependencies exist purely to decrease the MSRV, but what if I'm not particularly concerned about the MSRV or am happy with whatever the MSRV would otherwise be? It would be great to shed the 4 (transitive) dependencies if I'm not benefiting from them.

The obvious approach is to feature gate these, perhaps via an aptly named compatibility feature. I would advocate for disabling the feature by default (because otherwise it's too easy for an indirect dependency to inadvertently reintroduce the dependencies), documenting the MSRV, and noting that it can be reduced by enabling the feature. I'm happy to implement this, too!

Edit: After taking a closer look, it seems that bringing the MSRV down to 1.48 without any dependencies would be rather straightforward except for perhaps a single function. For 1.48, we'd need:

  • unsigned_abs - this is simply wrapping_abs() as $T.
  • strip_prefix - this has a straightforward implementation as well
  • split_once - this can be replaced with find() + get(..) or indexing

That only leaves Duration::checked_div() to re-implement in 1.48. That being said, I can't get the compiler to complain about the function in 1.48; why is it marked as being const only in 1.52? This uses checked_div, which is only const on 1.52 and as far as I can tell, is impossible to implement as const externally. Nevertheless, it seems that the MSRV without standback and const_fn is 1.46. Given that we can get to 1.48 without much effort, I would actually advocate for abandoning the two dependencies entirely, opting instead for an MSRV of 1.48. And, if checked_div being const is really important, we can depend version_check and enable a cfg as needed.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 29, 2021

Given that time us widely used, I think having the MSRV be that little bit lower is worthwhile, at least for now. After all, it's still a fourteen release bump from 0.2, which supports Rust 1.32.

I would like to state for the record that I will be tracking the six-month timeframe quite closely in the future though. I keep track of a number of unstable and recently stabilized features here along with the date on which time can use them per policy. I currently have (in my head) an MSRV bump to 1.51 scheduled for the end of September, as that will unlock a fair amount (particularly with const generics internally). Until that time, though, the dependencies are still lightweight compared to most libraries.

I also just realized I goofed and didn't bump time-macros to a proper 0.2, so I will redo the release on the repo. I'm not going to bother with the full powerset, as nothing is actually changing.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 29, 2021

Welp, new release means new lints that I should probably fix before publishing! It'll be out tonight.

@SergioBenitez
Copy link

SergioBenitez commented Jul 29, 2021

Given that time us widely used, I think having the MSRV be that little bit lower is worthwhile, at least for now. After all, it's still a fourteen release bump from 0.2, which supports Rust 1.32.

It's exactly because time is widely used that you should attempt to decrease the number of dependencies as much as possible. Here's the impact after making the changes I suggested, increasing the MSRV a single point release from 1.46 to 1.47:

~/Local/time ❯ time cargo build
   Compiling version_check v0.9.3
   Compiling const_fn v0.4.8
   Compiling easy-ext v0.2.9
   Compiling standback v0.3.5
   Compiling time v0.3.0 (/Local/time)
    Finished dev [unoptimized + debuginfo] target(s) in 6.73s
cargo build 6.01s user 0.81s system 99% cpu 6.824 total
~/Local/time ❯ time cargo build                                                                                                                                                                                                                    main ●
   Compiling time v0.3.0 (/Local/time)
    Finished dev [unoptimized + debuginfo] target(s) in 1.43s
cargo build 1.38s user 0.15s system 100% cpu 1.529 total

That's a 4.7x speedup! I cannot imagine justifying a 4.7x slowdown to decrease the MSRV by a single point release.

@SergioBenitez
Copy link

If the .1 really matters, we can still do better by using version_check. I also tried rustcversion, but it triples the build time instead of just doubling it:

❯ time cargo build
   Compiling rustversion v1.0.5
   Compiling time v0.3.0 (/Local/time)
    Finished dev [unoptimized + debuginfo] target(s) in 3.43s
cargo build 3.11s user 0.42s system 99% cpu 3.529 total
❯ time cargo build
   Compiling version_check v0.9.3
   Compiling time v0.3.0 (/Local/time)
    Finished dev [unoptimized + debuginfo] target(s) in 2.46s
cargo build 2.29s user 0.27s system 100% cpu 2.561 total

@jhpratt
Copy link
Member Author

jhpratt commented Jul 30, 2021

Huh, didn't realize the dependencies added that much to compile time. Given that information, I'll bump MSRV to 1.48. 1.49 is the highest we could go given policy, so it's still in line with that.

@jhpratt
Copy link
Member Author

jhpratt commented Jul 30, 2021

Time 0.3.0 has been released.

@jhpratt jhpratt closed this as completed Jul 30, 2021
@jhpratt jhpratt unpinned this issue Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-core Area: anything not otherwise covered A-formatting Area: formatting A-macros Area: macros A-parsing Area: parsing C-cleanup Category: cleanup of existing code C-seeking-input 📣 Category: community input is desired C-tracking-issue Category: tracking issue for a feature/release
Projects
None yet
Development

No branches or pull requests

10 participants