[#654] Get week start and end days #666

sestrella · 2022-03-29T07:41:27Z

Thanks for contributing to chrono!

Have you added yourself and the change to the changelog? (Don't worry
about adding the PR number)
If this pull request fixes a bug, does it add a test that verifies that
we can't reintroduce it?

djc · 2022-03-29T07:44:26Z

Thanks for working on this!

I think I suggested to make these NaiveDateTime instead of NaiveDate, are you still planning to change that?

src/naive/isoweek.rs

sestrella · 2022-03-29T17:57:48Z

Regarding:

I think I suggested to make these NaiveDateTime instead of NaiveDate, are you still planning to change that?

Sorry, I was about to leave a message explaining why I choose a NaiveDate over a NaiveDateTime. NaiveDateTime works well for the start_day and end_day functions, unfortunately, the week coming from bounds(Weekday) cannot be represented as a range of NaiveDateTime since there is no Step trait to create a Range<NaiveDateTime> and the internal iterator NaiveDateDaysIterator only works for NaiveDate. From the consumer standpoint, I think it would be useful if bounds(Weekday) returns something that could be treated as Range or Iterator (NaiveDateDaysIterator) in that sense NaiveDate might be a good choice for the start_day and end_day functions since they defined the bounds of the week for bounds(Weekday), I'm thinking about the following use cases:

Looping over the days in a week
Checking if a day is part of a week (this could be done via contains which is part of Range)

What do you think?

djc · 2022-03-30T09:21:30Z

That makes sense to me, thanks for explaining your position in more detail!

sestrella · 2022-04-22T02:22:07Z

@djc I think this PR is finally ready for a full review. Would you mind taking a look at it, please?

sestrella · 2022-05-04T17:01:19Z

The implementation of start_day and end_day in the context of week vastly variate from the original proposal included in the issue, while most of the agreements made are document as comments, I'm more than happy to sum those up in order make it easier to review this PR. Please let me know if there is something I could do to facilitate the reviewing process. I deeply appreciate the time already invested in this PR

sestrella · 2022-05-05T21:53:49Z

The issue reported by the CI was fixed.

djc

Sorry for the delay in reviewing this -- I was on holiday yesterday.

I think it mostly looks good, but I have some feedback to consider.

djc · 2022-05-10T09:24:05Z

src/lib.rs

@@ -450,13 +448,15 @@ pub mod prelude {
    #[doc(no_inline)]
    pub use crate::SubsecRound;
    #[doc(no_inline)]
+    pub use crate::{Date, Week};


I don't think these need to be added to the prelude, since they're relatively niche.

src/traits.rs

djc · 2022-05-10T09:28:09Z

src/traits.rs

+    /// [start_day]: ./trait.Weeklike.html#tymethod.start_day
+    /// [end_day]: ./trait.Weeklike.html#method.end_day
+    #[inline]
+    fn range(&self) -> RangeInclusive<Self::Day> {


I think I'd prefer to call this days() to make it clear what the range's Item is. What do you think?

src/naive/date.rs

djc · 2022-05-10T09:30:20Z

src/naive/date.rs

+#[derive(Debug)]
+pub struct NaiveWeek {
+    date: NaiveDate,
+    weekday: Weekday,


I think weekday should be called start, as that is the intended role within the type (as far as I understand).

djc · 2022-05-10T09:31:17Z

src/naive/date.rs

+
+    #[inline]
+    fn start_day(&self) -> Self::Day {
+        let start = self.date.weekday().num_days_from_monday() as i64;


We could calculate this in u8, right? Why are we casting here?

djc · 2022-05-10T09:31:44Z

src/naive/date.rs

+        let start = self.date.weekday().num_days_from_monday() as i64;
+        let end = self.weekday.num_days_from_monday() as i64;
+        let days = start - end;
+        self.date - Duration::days(if days >= 0 { days } else { 7 + days })


I think if days >= 0 { days } else { 7 + days } could be more naturally expressed with some kind of modulo?

sestrella · 2022-05-27T19:16:54Z

@djc the latest commit seems to fix the CI issue, unfortunately the build is not getting trigger without an approval:

However, I tried it locally and it works:

djc · 2022-06-08T08:35:25Z

src/date.rs

 use crate::DateTime;
 use crate::{Datelike, Weekday};

+/// A week represented by a [`NaiveWeek`] + [Offset](crate::offset::Offset).
+#[derive(Debug)]
+pub struct Week<Tz: TimeZone> {


So, after reviewing some other PRs I'm on the fence about this concept of types that don't point to a specific time but do reference a timezone. Since the type represents a time span, the timezone offset could conceivably be different at the start of the span vs the end of the span, which we cannot represent accurately.

How important is having this type included to you? Could we make do with just the NaiveWeek?

@djc - It makes sense to me, starting with NaiveWeek sounds fair. I just updated the PR simplifying the implementation a little bit.

sestrella · 2022-06-09T16:44:10Z

Pushed a PR addressing the error reported by the linter:

> cargo fmt -- --check --color=always && echo "OK"
OK

djc · 2022-06-13T08:48:21Z

Still some doc link issues, can you check that out?

Co-authored-by: David Mazarro <dmunuera@stackbuilders.com> Co-authored-by: Jorge Guerra <jguerra@stackbuilders.com>

sestrella · 2022-06-15T05:59:45Z

Hopefully, everything turns green this time 🤞🏼

> cargo +nightly doc && echo "OK"
OK

djc · 2022-06-15T12:01:35Z

Thanks for seeing this through!

sestrella · 2022-06-15T13:40:20Z

Thank you for all the feedback and time spent on this PR

DavidMazarro · 2022-06-15T14:04:35Z

Closes #654

sestrella commented Mar 29, 2022

View reviewed changes

src/naive/isoweek.rs Outdated Show resolved Hide resolved

sestrella force-pushed the week_start_and_end_days branch 3 times, most recently from f76fa06 to 4c7e6df Compare April 22, 2022 02:19

sestrella marked this pull request as ready for review April 22, 2022 02:19

sestrella force-pushed the week_start_and_end_days branch 2 times, most recently from 7ddf118 to 38fdb29 Compare May 5, 2022 21:44

djc reviewed May 10, 2022

View reviewed changes

sestrella force-pushed the week_start_and_end_days branch 2 times, most recently from 785dd1c to b274146 Compare May 17, 2022 05:17

sestrella force-pushed the week_start_and_end_days branch from 0f9f5ba to b8d99d7 Compare May 26, 2022 04:45

djc reviewed Jun 8, 2022

View reviewed changes

sestrella force-pushed the week_start_and_end_days branch 2 times, most recently from 502df87 to 725e6d9 Compare June 9, 2022 16:40

sestrella and others added 4 commits June 15, 2022 00:31

[chronotope#654] Get week start and end days

f48b054

Co-authored-by: David Mazarro <dmunuera@stackbuilders.com> Co-authored-by: Jorge Guerra <jguerra@stackbuilders.com>

Update CHANGELOG

c9a9dc9

Fix formatting issue

9c87078

Fix broken links

f4f73f6

sestrella force-pushed the week_start_and_end_days branch from 725e6d9 to f4f73f6 Compare June 15, 2022 05:53

djc merged commit 8dd7245 into chronotope:main Jun 15, 2022

sestrella deleted the week_start_and_end_days branch June 15, 2022 13:39

DavidMazarro mentioned this pull request Jun 27, 2022

Add functions to get start and end for other kinds of time periods #722

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#654] Get week start and end days #666

[#654] Get week start and end days #666

sestrella commented Mar 29, 2022 •

edited

djc commented Mar 29, 2022

sestrella commented Mar 29, 2022

djc commented Mar 30, 2022

sestrella commented Apr 22, 2022

sestrella commented May 4, 2022

sestrella commented May 5, 2022

djc left a comment

djc May 10, 2022 •

edited

djc May 10, 2022

djc May 10, 2022

djc May 10, 2022

djc May 10, 2022

sestrella commented May 27, 2022 •

edited

djc Jun 8, 2022

sestrella Jun 9, 2022

sestrella commented Jun 9, 2022 •

edited

djc commented Jun 13, 2022

sestrella commented Jun 15, 2022

djc commented Jun 15, 2022

sestrella commented Jun 15, 2022

DavidMazarro commented Jun 15, 2022

[#654] Get week start and end days #666

[#654] Get week start and end days #666

Conversation

sestrella commented Mar 29, 2022 • edited

Thanks for contributing to chrono!

djc commented Mar 29, 2022

sestrella commented Mar 29, 2022

djc commented Mar 30, 2022

sestrella commented Apr 22, 2022

sestrella commented May 4, 2022

sestrella commented May 5, 2022

djc left a comment

Choose a reason for hiding this comment

djc May 10, 2022 • edited

Choose a reason for hiding this comment

djc May 10, 2022

Choose a reason for hiding this comment

djc May 10, 2022

Choose a reason for hiding this comment

djc May 10, 2022

Choose a reason for hiding this comment

djc May 10, 2022

Choose a reason for hiding this comment

sestrella commented May 27, 2022 • edited

djc Jun 8, 2022

Choose a reason for hiding this comment

sestrella Jun 9, 2022

Choose a reason for hiding this comment

sestrella commented Jun 9, 2022 • edited

djc commented Jun 13, 2022

sestrella commented Jun 15, 2022

djc commented Jun 15, 2022

sestrella commented Jun 15, 2022

DavidMazarro commented Jun 15, 2022

sestrella commented Mar 29, 2022 •

edited

djc May 10, 2022 •

edited

sestrella commented May 27, 2022 •

edited

sestrella commented Jun 9, 2022 •

edited