-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
replace Date::to_iso_week_date
with Date::iso_week_date
#218
Labels
breaking change
Issues that require a breaking change for resolution.
Comments
BurntSushi
added a commit
that referenced
this issue
Jan 25, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
BurntSushi
added a commit
that referenced
this issue
Jan 25, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
BurntSushi
added a commit
that referenced
this issue
Jan 27, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
BurntSushi
added a commit
that referenced
this issue
Jan 30, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
BurntSushi
added a commit
that referenced
this issue
Feb 1, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
BurntSushi
added a commit
that referenced
this issue
Feb 2, 2025
These were deprecated. Users should use `Date::iso_week_date` and `ISOWeekDate::date` instead. Fixes #218
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
And also, replace
ISOWeekDate::to_date
withISOWeekDate::date
.The
to_
variants were deprecated injiff 0.1
.I made this change because I felt like
Zoned::iso_week_date
made more sense thanZoned::to_iso_week_date
, and was more consistent withZoned::date
. I then bubbled this change down toDate
so that the APIs were named consistently betweenZoned
andDate
.Jiff arguably doesn't use the
to_
prefix perfectly consistently across the crate, and is also somewhat inconsistent with the Rust API guidelines on naming. The guidelines are a little troublesome in this context because they depend, somewhat, on the "cost" of the conversion methods.Zoned::date
, for example, is effectively free (it's aCopy
of a type that is 4 bytes big). ButZoned::iso_week_date
is not, since there is a conversion from one calendar system to another happening. Moreover,Zoned::date
was not always effectively free in my initial design, but became such by the first release. In theory it could become more expensive in a future release, but probably not.This situation repeats itself in a few other places. Basically, Jiff blurs the line between methods that do work to return a value and methods that return a value as-is. I think following the naming convention here rigorously would end up with a naming scheme that is seemingly arbitrary, where some methods have
to_date
while others havedate
and so on. And the naming could end up becoming wrong if the implementation details change.One downside here though is that one cannot use the naming to get a sense of which methods are "cheap" and which are "not cheap." I think probably in most cases it won't matter, and folks will just need to rely on profiling to determine when a method call should be amortized.
The text was updated successfully, but these errors were encountered: