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

now() functionality for FixedOffest and a Utc::fixed_now() for DateTime w/ TimeZone (set to 0) #829

Closed
wants to merge 30 commits into from

Conversation

epipheus
Copy link

Trying this again, forgive me if this is annoying, I'm new to Rust, so you'll have to review to make sure I didn't screw anything up.

added a now() function to Offset trait and a fixed_now() function to Utc that calls the trait's now. FixedOffset's now() function get the duration from the system the same way Utc::now does but afterwards uses timestamp_opt to return DateTime.

This was in response to related comment in #624 and #169

esheppa and others added 30 commits August 29, 2022 10:52
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
Co-authored-by: Dirkjan Ochtman <dirkjan@ochtman.nl>
…ules

The internal structs can be moved into their respective modules
because `pub(super)` is now supported by the MSRV.
As discussed in chronotope#263. Note that this breaks existing code that did not
explicitly specify the offset in calls to `DateTime::parse_from_*`, as
they are now ambiguous.

Relies on chronotope#271 for conversion back into `DateTime<Utc>` from
`DateTime<FixedOffset>` as the latter is used under the hood.
Clarify the behavior of the parse methods, the relationship between ISO 8601 and
RFC 3339, and use a brief description on the first line of each function's
rustdoc to keep the resulting documentation pretty and concise.
Construct NaiveDateTime from millis since epoch
…out now yields a warning of being unused despite action erroring because it's missing
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to at least one test.

And remember to target the 0.4.x branch instead of main, as mentioned in the previous PR.

@@ -3,7 +3,14 @@

//! The UTC (Coordinated Universal Time) time zone.

use super::{FixedOffset, LocalResult, Offset, TimeZone};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't make unrelated changes (the imports are sorted and split this way on purpose).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apologies, overbearing IDE.

@@ -194,6 +194,9 @@ impl<T: fmt::Debug> LocalResult<T> {
pub trait Offset: Sized + Clone + fmt::Debug {
/// Returns the fixed offset from UTC to the local time stored.
fn fix(&self) -> FixedOffset;
/// Returns a fixed offset DateTime at the current date and time in self's timezone
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should add this to the trait.

@@ -6,15 +6,17 @@
use core::fmt;
use core::ops::{Add, Sub};

use num_integer::div_mod_floor;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the unrelated import moves.

@@ -102,6 +107,10 @@ impl Offset for Utc {
fn fix(&self) -> FixedOffset {
FixedOffset::east(0)
}
#[cfg(feature = "std")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think adding this makes sense.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If FixedOffset had it's own now that didn't need Utc but required systemtime which is in std...

@@ -128,6 +130,12 @@ impl Offset for FixedOffset {
fn fix(&self) -> FixedOffset {
*self
}
#[cfg(feature = "std")]
fn now(&self) -> DateTime<FixedOffset> {
let now =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about writing this as Utc::now().with_timezone(self)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started that way then eventually wondered if FixedOffset should have it's own now()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh. i understand. yes.

@epipheus
Copy link
Author

I'll get it right eventually. Apologies.

@epipheus epipheus changed the base branch from main to 0.4.x September 30, 2022 05:22
@esheppa
Copy link
Collaborator

esheppa commented Oct 1, 2022

Hi @epipheus - thanks for setting up a PR for this - one suggestion, you could squash all your commits together using git rebase -i. It looks like some extra commits have also been added in here due to the change of base branch, so if you squash your commits, you can then cherry-pick that resulting commit into a new branch and PR which can directly target 0.4.x.

In terms of the code, another potential option is extending the TimeZone trait, instead of the Offset trait, with something like:

pub trait TimeZone: Sized + Clone {
    // other methods skipped for brevity
    #[cfg(feature = "clock")]
    /// Get the current time in the specified timezone
    fn now(&self) -> DateTime<Self> {
        Self::from_utc_datetime(self, &Utc::now().naive_utc())
    }
}

this should enable things like:

Utc.now() // (same as `Utc::now()`)
Local.now() // (same as `Local::now()`)
FixedOffset::east(3600).now()
chrono_tz::Pacific::Auckland.now()

In terms of converting from Utc to FixedOffset - this is a little tricky as FixedOffset is a bit special in that it maintains state (the offset), so this either needs to be one of:

  • special case inherent method on DateTime (probably the best option as it seems like it might be a commonly used function?), or
  • use an instance of FixedOffset::east(0) and then one of:
    • FixedOffset::east(0).from_utc_datetime(utc_datetime.naive_utc())
    • utc_datetime.with_timezone(&FixedOffset::east(0))

However I think there is a reasonable solution that could solve this generically as a inherent impl on DateTime:

/// Converts the timezone to just the current offset throwing away any extra information
impl<Tz: TimeZone> DateTime<Tz> {
    pub fn with_fixed_offset(self) -> DateTime<FixedOffset> {
        self.with_timezone(&self.offset.fix())
    }
}

@epipheus
Copy link
Author

epipheus commented Oct 2, 2022

Thanks for much for the comments and thanks for bearing with me, I am far from awesome in Rust but doing something will hopefully get me there. I never knew anything about cherry-pick so this is cool, I was going to just start over from a fresh fork especially since I disturbed import order, etc.

Will attempt doing what you said without messing up after the weekend.

Zomtir added a commit to Zomtir/chrono that referenced this pull request Mar 22, 2023
- Creates a global Error enum
- Breaks backwards compatiblility mainly because of promoting fallable functions (chronotope#263)
- Some tests still fall
- Not all doctests are fixed
- to_naive_datetime_with_offset function is broken and needs fixing
- serde related stuff is not checked properly

This is a rebase of chronotope#817 onto the 0.5 main branch. Main differences:

- Unify three different error structs
- Removed ErrorKind
- Adapted a lot of unit tests
- Removed some commits from presumably unrelated branches (chronotope#829) or
  mainlined commits (chronotope#271)

Co-authored-by: John-John Tedro <udoprog@tedro.se>
@pitdicker
Copy link
Collaborator

@epipheus Is this something you still want to work on?

@epipheus
Copy link
Author

Apologies for massive delay. At the time had a death in the family and I definitely completely fell off for quite a while. I also thought I saw a recent commit that perhaps made this moot. Happy to take a look.

@pitdicker
Copy link
Collaborator

I am sorry to hear to hear that.

To be honest I don't know how much we really need this feature with recent commits.
Some examples of what we can currently write:

let now_utc = Utc::now();
let local_now = Local::now();

// `DateTime<FixedOffset>` with offset = +00:00
let now_utc_fixed = Utc::now().fixed_offset();
// or
let now_utc_fixed: DateTime<FixedOffset> = Utc::now().into();

// `DateTime<FixedOffset>` with offset of system timezone
let local_now_fixed = Local::now();
// or
let local_now_fixed: DateTime<FixedOffset> = Local::now().into();

// Convert to another timezone:
let offset = FixedOffset::east_opt(5 * 60 * 60).unwrap();
let now_with_offset = Utc::now().with_timezone(&offset);

It don't think adding now() to the TimeZone trait as suggested by @esheppa would be a good idea. It would cause the available methods of the trait to change depending on whether the clock features is enabled.

If you want to add something, maybe adding it to DateTime<Tz> would be a nice place to make this functionality available for all timezones.

@epipheus
Copy link
Author

This feels much better. Maybe we can just close this with current commits really covering this well.

@epipheus
Copy link
Author

Sure. Adding to time zones would be nice.

@pitdicker
Copy link
Collaborator

pitdicker commented Jul 20, 2023

If you want to add something, maybe adding it to DateTime<Tz> would be a nice place to make this functionality available for all timezones.

I thought it would be possible to add something like this:

impl<Tz: TimeZone> DateTime<Tz> {
    /// Returns a `DateTime` which corresponds to the current date and time.
    #[cfg(feature = "clock")]
    #[must_use]
    pub fn now() -> DateTime<Tz> {
        Utc::now().into()
    }
}

But that doesn't work. We have no way to generically construct a DateTime with some arbitrary type that implements TimeZone.

Maybe we can just close this with current commits really covering this well.

Okay. I'll close this PR, but will open one to add the examples from #829 (comment) to the documentation somewhere.
Edit: see #1192.

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.

8 participants