add Day<Tz> #883
add Day<Tz> #883
Conversation
src/day.rs
Outdated
| /// | ||
| pub fn succ(&self) -> Option<Day<Tz>> { | ||
| Some(Day { date: self.date.succ_opt()?, tz: self.tz }) | ||
| } | ||
|
|
||
| /// | ||
| pub fn pred(&self) -> Option<Day<Tz>> { | ||
| Some(Day { date: self.date.pred_opt()?, tz: self.tz }) | ||
| } |
There was a problem hiding this comment.
As mentioned elsewhere, I kinda want to move away from providing these...
There was a problem hiding this comment.
I've removed, however these are potentially useful when dealing with non-instant style types, for example, my_day.succ().start() is an exclusive end time for my_day - but this could be implemented directly
| } | ||
|
|
||
| /// | ||
| pub fn start(&self) -> DateTime<Tz> { |
There was a problem hiding this comment.
Could use some documentation of why we have to use a loop here. What problem is this trying to solve?
There was a problem hiding this comment.
I've added some more documentation here including around panics and incorrect results
There was a problem hiding this comment.
One way to have this not require heuristics would be to make the loop slightly differently:
- first, try with NaiveTime::MIN as a fast path
- then, for each 15min segment,
- if MIN + segment_end is not ok, go to the next segment
- else, if MIN + segment_end - 1ns is ok too, do a dichotomy between MIN + segment_begin and MIN + segment_end
That said, I personally don't think that doing this would be necessary. Summer time changes across midnight are unlikely enough already, having them not be a multiple-of-15-min offset would be actively harmful to basically any software.
Another idea would be, to delegate the implementation of this to a new trait method on TimeZone, that would have a default implementation of adding NaiveTime::MIN; and on the day chrono_tz gets a summer time transition across midnight the specialization can be added there.
I'm personally partial towards the second idea, as it gets us all of the simplest implementation right now, the most efficient one, and the most future-proof one. Does that make sense?
|
|
||
| impl<Tz> Eq for Day<Tz> where Tz: TimeZone + Copy + Display {} | ||
|
|
||
| impl<Tz> Day<Tz> |
There was a problem hiding this comment.
Style nit: would prefer to have the inherent impl before trait impls.
Also I would write impl<Tz: TimeZone + Copy + Display> instead of having the where clause`, since it's a bit more concise.
| /// | ||
| pub fn checked_add_days(self, days: Days) -> Option<Self> { | ||
| if days.0 == 0 { | ||
| return Some(self); | ||
| } | ||
|
|
||
| i64::try_from(days.0).ok().and_then(|d| self.diff_days(d)) | ||
| } | ||
|
|
||
| /// | ||
| pub fn checked_sub_days(self, days: Days) -> Option<Self> { | ||
| if days.0 == 0 { | ||
| return Some(self); | ||
| } | ||
|
|
||
| i64::try_from(days.0).ok().and_then(|d| self.diff_days(-d)) | ||
| } | ||
|
|
||
| fn diff_days(self, days: i64) -> Option<Self> { | ||
| let date = self.date.checked_add_signed(Duration::days(days))?; | ||
| Some(Day { date, ..self }) | ||
| } |
There was a problem hiding this comment.
I feel like in general we should start with a minimal API here, we can grow it over time as people actually ask for it.
There was a problem hiding this comment.
Agreed, however I added these specifically to use here https://github.com/esheppa/db-dump/blob/7350bfabb9ff48b999ba4d6b0e827e8e2eea275c/examples/industry-coefficient.rs#L37
esheppa
force-pushed
the
add-day-tz
branch
2 times, most recently
from
bedd6eb to
7169602
Compare
|
(FWIW, I just linked a fuzzer crash from #948 ; which needs implementing basically this to fix it) |
|
As a summary: The type In my opinion this type is not generally useful enough to be added. Also we have not had others except for the one use case in that comment ask for this functionality. |
|
Closing this PR. If someone wants to push for this feature, please open an issue. |
No description provided.