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

Consistently name methods that turn into another more complete type #1527

Open
pitdicker opened this issue Mar 18, 2024 · 8 comments
Open

Consistently name methods that turn into another more complete type #1527

pitdicker opened this issue Mar 18, 2024 · 8 comments

Comments

@pitdicker
Copy link
Collaborator

Continuing from #1523.

How do we want to consistently name methods that turn one type into another more complete type?
Currently we have little consistency, starting with and_, with_, from_, or nothing.

chrono time-rs proposal
NaiveDate::and_time Date::with_time NaiveDate::at
NaiveDate::and_hms Date::with_hms NaiveDate::at_hms
NaiveDate::and_hms_milli Date::with_hms_milli NaiveDate::at_hms_milli
NaiveDate::and_hms_micro Date::with_hms_micro NaiveDate::at_hms_micro
NaiveDate::and_hms_nano Date::with_hms_nano NaiveDate::at_hms_nano
NaiveDateTime::and_local_timezone PrimitiveDateTime::assume_offset NaiveDateTime::in_timezone
NaiveDateTime::and_utc PrimitiveDateTime::assume_utc NaiveDateTime::in_utc
TimeZone::with_ymd_and_hms TimeZone::at_ymd_and_hms
TimeZone::timestamp TimeZone::at_timestamp
TimeZone::timestamp_millis TimeZone::at_timestamp_millis
TimeZone::timestamp_micros TimeZone::at_timestamp_micros
TimeZone::timestamp_nanos TimeZone::at_timestamp_nanos
TimeZone::from_local_datetime TimeZone::at_local
TimeZone::from_utc_datetime TimeZone::at_utc

Eos simply uses at to combine a date with a time.
I have to admit at_ and in_ read pretty nice (combined with macros and ?):

let today = Local.now().date();
let appointment = today.at_hms(13, 0, 0)?;
// or
let noon = time!(13:00:00);
let appointment = today.at(noon);

let departure = datetime!(2024-03-18 11:15:00).in_timezone(New_York)?;
let arrival = Amsterdam.at_ymd_and_hms(2024, 3, 18, 23, 55, 0)?;
// or
let departure = New_York.at_local(datetime!(2024-03-18 11:15:00))?;

let log_time = Local.at_timestamp(some_ts)?;

We can also rename them all to with_*. However we have 26 methods that currently follow a convention that with_ keeps the type the same and modifies some component of the date/time. It seems nice to maintain a distinction.

@djc
Copy link
Contributor

djc commented Mar 18, 2024

We can also rename them all to with_*. However we have 26 methods that currently follow a convention that with_ keeps the type the same and modifies some component of the date/time. It seems nice to maintain a distinction.

Hmmm, not sure I want to overindex on maintaining this distinction? It's one factor, but not sure how important it should be.

Can you add another table that with these with_ methods to we can compare?

@pitdicker
Copy link
Collaborator Author

Those would be:

NaiveDate::with_year
NaiveDate::with_month
NaiveDate::with_day
NaiveDate::with_ordinal

NaiveTime::with_hour
NaiveTime::with_minute
NaiveTime::with_second
NaiveTime::with_nanosecond

NaiveDateTime::with_year
NaiveDateTime::with_month
NaiveDateTime::with_day
NaiveDateTime::with_ordinal
NaiveDateTime::with_hour
NaiveDateTime::with_minute
NaiveDateTime::with_second
NaiveDateTime::with_nanosecond

DateTime::with_year
DateTime::with_month
DateTime::with_day
DateTime::with_ordinal
DateTime::with_time
DateTime::with_hour
DateTime::with_minute
DateTime::with_second
DateTime::with_nanosecond
DateTime::with_timezone

Time-rs uses replace_ for most of these.

@djc
Copy link
Contributor

djc commented Mar 18, 2024

Okay, I think it sounds good to stick with with_ for same-type and use at_/in_ for different-type. Let's introduce the new names on main with deprecations?

@pitdicker
Copy link
Collaborator Author

Wow, that is a fast decision 😄. But I think it is going to be a nice improvement.

Let's introduce the new names on main with deprecations?

Those deprecations are going to hit everyone who uses chrono. I am afraid at some point we are going to chaise users away if we keep causing too much churn.

@djc
Copy link
Contributor

djc commented Mar 18, 2024

I mean, it will still be churn if we change them only in 0.5, except that will be a lot more churn all at the same time? Better to smooth it out over time (and give us a chance to get feedback on the change), IMO. Maybe we can split these changes out?

BTW I do kind of like the assume_ prefixes from time-rs because they IMO more clearly/explicitly convey the intended semantics. So maybe we should copy those instead of in_()?

@pitdicker
Copy link
Collaborator Author

pitdicker commented Mar 18, 2024

I mean, it will still be churn if we change them only in 0.5, except that will be a lot more churn all at the same time? Better to smooth it out over time (and give us a chance to get feedback on the change), IMO.

I can't deny it is going to have to happen at some point. And from our perspective doing it on the 0.4 branch brings advantages.

But I really do think there is a limit to what at least a portion of our users will tolerate. And I don't want to find out where that limit lays.
If we deprecate methods because they can lead to easy mistakes or unexpected panics that seems okay to do. Even users that are annoyed by the deprecation warnings can see it is at least done for a good reason.
Doing deprecations because we consider a new name to be nicer is going to be a much harder sell.

Also duplicating 13 methods is not going to make our documentation any clearer.

The difficult part here is that I am arguing for not doing it on 0.4 while personally I don't mind it much if changes are smoothed out over time.

@djc
Copy link
Contributor

djc commented Mar 18, 2024

Okay, well, let's skip 0.4.x for now then.

@pitdicker
Copy link
Collaborator Author

BTW I do kind of like the assume_ prefixes from time-rs because they IMO more clearly/explicitly convey the intended semantics. So maybe we should copy those instead of in_()?

I'd like to test both and see how it looks in some sample code.

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

No branches or pull requests

2 participants