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

and_local_timezone naming #870

Open
Ten0 opened this issue Nov 13, 2022 · 5 comments
Open

and_local_timezone naming #870

Ten0 opened this issue Nov 13, 2022 · 5 comments
Labels
API-incompatible Tracking changes that need incompatible API revisions

Comments

@Ten0
Copy link

Ten0 commented Nov 13, 2022

Hello,

I have a question about the naming of this function.

I don't understand what information the local is supposed to bear. What is a "local timezone" compared to a "timezone"?

I'm not particularly familiar with all the timezone-related science, but I think I would have named it simply NaiveDateTime::in_timezone -> DateTime<Tz> (although I would understand with_timezone as well).

How is the current naming better?

Thanks,

@djc
Copy link
Member

djc commented Nov 14, 2022

I think the issue here is that there are two different meanings for combining a NaiveDateTime and a TimeZone:

  • NaiveDateTime is already in TimeZone, clarify this in the type system
  • NaiveDateTime is actually in Utc, need to add/subtract an offset to bring it into TimeZone

This is the reason for the _local bit. I think the and_ prefix vs with_ is simply because this is the convention in chrono, which possibly predates widespread usage of with_ in the rest of the ecosystem.

@Ten0
Copy link
Author

Ten0 commented Nov 14, 2022

Thank you!

IMO:

  • naive_datetime.and_timezone(tz) could hardly be misinterpreted as "Assume this is Utc, then convert it to provided timezone".
  • It would seem best (not sure if that's already the case) if the only way to bring a naive datetime that "is actually Utc" in an arbitrary timezone was to first assert it's in Utc by turning it into a timezone aware datetime, and then use the usual timezone conversion features. I don't think that is ever significantly more boilerplatish than having a function that does both, and seems always easier to read and consequently less error-prone.

That plus the fact the "local" does induce confusion (at least it did for me), I would tend to favor and_timezone.

@djc
Copy link
Member

djc commented Nov 15, 2022

For context, see #711. @esheppa any opinions here?

@Ten0
Copy link
Author

Ten0 commented Nov 15, 2022

So we've been discussing this naming at my workplace for about 30min (while choosing naming for helpers we have written to handle fallbacks to either later or earlier time around DST changes), and we concluded that the clearest naming would be naive_date.assume_in_timezone(tz) (or assume_timezone) (as in "assume the following datetime is in fact in this timezone, as determined by prior logic or knowledge").

That clearly gives the intent for what may be confused (in particular makes it particularly hard to confuse it with the UTC conversion version).

(Internally, we now use naive_date.assume_in_timezone_fallback_earlier(tz) and naive_date.assume_in_timezone_fallback_later(tz) in an extension trait, and we also have an infaillible helper for the UTC version: naive_date.assume_in_utc()).

Wdyt?

@esheppa
Copy link
Collaborator

esheppa commented Nov 16, 2022

and we concluded that the clearest naming would be naive_date.assume_timezone(tz)

This sounds good to me! One option we had thought about originally was assert_timezone but I much prefer assume_timezone.

Internally, we now use naive_date.assume_timezone_fallback_earlier(tz) and naive_date.assume_timezone_fallback_later(tz)

The plan is to eventually have some methods for this within chrono, you can see discussions here #716 and here #830. Would appreciate feedback or alternative thoughts on the ideas there

@pitdicker pitdicker mentioned this issue Apr 27, 2023
@pitdicker pitdicker added the API-incompatible Tracking changes that need incompatible API revisions label Jul 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-incompatible Tracking changes that need incompatible API revisions
Projects
None yet
Development

No branches or pull requests

4 participants