Years type implementation #900

Rafael-Conde · 2022-11-26T23:05:28Z

I started a simple implementation that I intend to add proper add checks once the problem is actually solved.

I implemented simples tests too, but there is still a lot to do. I'm currently opening the PR so discussions about the implementation can occur here and you guys can follow the progress. I believe this is semver compatible since I'm only adding a new type so I targeted the 0.4.x branch, hopefully, this type will make it into that version.

I already applied the recommendations @esheppa made in this comment on issue #416

Rafael-Conde · 2022-11-26T23:11:05Z

I believe I already made the mistake to "commit to main". I branched from main and that's the problem. I'll change that later because I don't have more time now. I decided to open the PR since I said I would. I'll fix that later, wether opening a new PR with the proper branch or another fix for this one.

One thing too, is there a deadline for the 0.4.x version to be out?

djc · 2022-11-28T08:54:41Z

There's no deadline! Thanks for working on this, I'll take a more detailed look after you rebase on 0.4.

I started a simple implementation that I intend to add proper add checks once the problem is actually solved.

Rafael-Conde · 2022-11-29T16:13:50Z

There's no deadline!

Nice! I'm likely to work on it on the weekends,

Thanks for working on this

Not a problem! I appreciate the opportunity to contribute!

I'll take a more detailed look after you rebase on 0.4.

Ok! I'm still in the middle of the implementation(I couldn't finish it on my first session), I wrote a few lines of code but basically it does nothing so far. But help is always welcome! you guys are very polite when helping out! Thank you for that!

Also I got a bit messed up while rebasing but I think everything should be in order right now.

If you guys want, I can leave a comment here once I got any of the implementations actually working. To review and improve the code.

Also, during development I plan to leave comments on the code I'm working on to try and complement the code with some thoughts. Other than that I could mark the lines of code here and leave the comment here, where I would otherwise leave in the code.

djc · 2022-12-01T10:21:19Z

I've converted this to a draft PR, please push the "Ready for review" button once you think it's ready!

Rafael-Conde · 2022-12-01T13:16:00Z

Ok! Thank you for that! I believe I should've done it from the beginning, I'm still learning haha

I would like to ask too, the checks in the end of the PR are only for my modifications? in order to merge, all of them should be ok, right?

djc · 2022-12-02T10:45:15Z

Yes, CI should pass. It's possible that some failures are unrelated to your changes, but should be unlikely.

esheppa

just leaving some initial comments, mostly on code location

esheppa · 2022-12-02T23:06:10Z

src/years.rs

+}
+
+impl NaiveDate {
+    pub fn checked_add_years(self, years: Years) -> Option<NaiveDate> {


this impl should be in naive/date.rs - you can import Years over there

esheppa · 2022-12-02T23:06:36Z

src/years.rs

+    pub fn checked_sub_years(self, num_years: Years) -> Option<NaiveDate> {}
+}
+
+// impl Add<Years> for NaiveDate {}


these impls should be in the module where the target type is defined

esheppa · 2022-12-02T23:08:11Z

src/years.rs

+}
+
+// almost surely there is a more precise way to implement this
+fn is_leap_year(year: i32) -> bool {


you can use ordinal flags and then use the ndays method to figure out whether the target year is a leap year or not. You can construct the ordinal flags directly from the target year itself

Thank you for the pointers! I'll surely make the requested changes! This one in special will be of great help! really appreciated!

I'll move the other impl blocks to the proper locations too!

…de/Years-impl

Rafael-Conde · 2022-12-17T22:49:50Z

I finished the implementation, it ended up a lot simpler than I first imagined but I currently don't see why not do that haha

I still need to do the documentation, but the code is compiling and the simple tests I wrote are working. (I made a simple TODO docs just to run but removed from the commit so I don't forget to do it once I'm back at it).

you can use ordinal flags and then use the ndays method to figure out whether the target year is a leap year or not. You can construct the ordinal flags directly from the target year itself

this helped a lot and ended up pointing me to the solution haha Thank you!!

I'm planning on implementing the addition of Years and I'm still thinking about how to get the difference, in complete years, between 2 dates.

That specific difference between dates helps me a lot.

PS: took me a while to get back to it because of life, but I should work more on it in the following days.

pitdicker

My personal opinion is that I don't care much for the Day and Month types. I prefer writing date.checked_add_years(5) over date.checked_add_years(Years::new(5)) or date + Years::new(5). It seems like over-using the type system to me. But others may like it 🤷.

The DateTime<Tz> does not have the extra methods yet.

pitdicker · 2023-07-06T05:03:13Z

src/date.rs

@@ -23,6 +23,8 @@ use crate::oldtime::Duration as OldDuration;
 use crate::DateTime;
 use crate::{Datelike, Weekday};

+use crate::Years;
+


The Date type is deprecated, better to not add new methods to it.

pitdicker · 2023-07-06T05:05:34Z

src/naive/date.rs

@@ -1248,6 +1250,23 @@ impl NaiveDate {
        NaiveWeek { date: *self, start }
    }

+    // I think self here is a copy, but I still have to make sure it's


It is a copy, you only want to take self by reference for DateTime<Tz>.

pitdicker · 2023-07-06T05:09:27Z

src/naive/date.rs

+            return Some(self);
+        }
+        let num_years = i32::try_from(years.0).ok()?;
+        self.with_year(self.year() + num_years)


This will fail if you add 1 year to February 29th. This is different from checked_add_months, which will fall back to the last day of the month. Shouldn't they work the same?

How do you handle integer overflow in for example self.year() + i32::MAX?

pitdicker · 2023-07-06T05:11:44Z

src/naive/datetime/mod.rs

+    pub fn checked_sub_years(mut self, num_years: Years) -> Option<NaiveDateTime> {
+        self.date = self.date.checked_sub_years(num_years)?;
+        Some(self)
+    }
 }


NaiveDateTime could also use Add and Sub implementations.

pitdicker · 2023-07-06T05:12:27Z

src/years.rs

@@ -0,0 +1,58 @@
+// I usually develop with this clippy flags
+// but I can remove once the this module is done


👍 when done.

pitdicker · 2023-07-06T05:12:52Z

src/years.rs

+        clippy::unwrap_used,
+        clippy::expect_used)]
+
+/// TODO


Needs documentation.

pitdicker · 2023-07-06T05:13:56Z

src/years.rs

+        clippy::expect_used)]
+
+/// TODO
+#[derive(Debug)]


Can you make this match the derives and attributes of Month?

pitdicker · 2023-07-06T05:25:49Z

src/naive/date.rs

+            return Some(self);
+        }
+        let num_years = i32::try_from(years.0).ok()?;
+        self.with_year(self.year() - num_years)


djc · 2023-07-06T07:06:09Z

Yeah, I've been on the fence about the proliferation of types here -- not sure it's the best idea, even though I may have started it. (I think the original idea was that it allowed for using the operators, which does seem like nice API.)

pitdicker · 2023-07-06T07:11:23Z

I was thinking we could add impl From<u32> for Month and make checked_add_month take Into<Month> to make the API a little nicer to use. And the same for Day.

We can stuff the types inside a module mostly out of sight 😄.

pitdicker · 2023-09-18T17:11:50Z

Closing due to inactivity, and because we may want to step away from Day and Month types, and Year as in this PR, in favour of a more feature-complete CalendarDuration type.

@Rafael-Conde Thank you for working on this!

Rafael-Conde · 2023-09-18T20:54:31Z

Thank you for notifying my @pitdicker!!

I recently came back to this PR and saw how much work you did!

I was really impressed, a lot of time and thought!!

One last recommendation as I leave this, I saw a(apparently old) discussion about what to do with invalid dates(e.g. adding a month to January 31st, should it be February 28 or March 02?)

For this matter I would recommend a cpp conference video of the creator of Chrono describing how he handled that providing invalid dates and methods for resolving it to the preferred way(it was possible to yield both results from the invalid date I mentioned in the example). I'll search for the link and I'll post here or maybe in the new PR, if you folks find it useful. Keep up the good work!!!

pitdicker · 2023-09-19T06:43:02Z

Thank you, and that would be interesting!

I suppose we have the capability to represent an invalid date with the internal Mdf type (and wrap into something nicer and public). But then we also have two other places where the result could become invalid: DST transitions and leap seconds. Something to think about 😄.

Rafael-Conde force-pushed the Conde/Years-impl branch from ba763b7 to 9bee716 Compare November 29, 2022 15:50

add Years to mod tree & exploring a simple impl

70da849

I started a simple implementation that I intend to add proper add checks once the problem is actually solved.

Rafael-Conde force-pushed the Conde/Years-impl branch from 9bee716 to 70da849 Compare November 29, 2022 15:57

djc marked this pull request as draft December 1, 2022 10:20

esheppa requested changes Dec 2, 2022

View reviewed changes

Rafael-Conde added 2 commits December 17, 2022 16:21

Merge branch '0.4.x' of https://github.com/chronotope/chrono into Con…

1a30f24

…de/Years-impl

finished impl, changed code position & clean up

b6167de

doomlaur mentioned this pull request Jan 9, 2023

Support i128 and extend limits for TimeDelta in chrono 0.5 #933

Closed

pitdicker requested changes Jul 6, 2023

View reviewed changes

pitdicker closed this Sep 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Years type implementation #900

Years type implementation #900

Rafael-Conde commented Nov 26, 2022

Rafael-Conde commented Nov 26, 2022

djc commented Nov 28, 2022

Rafael-Conde commented Nov 29, 2022

djc commented Dec 1, 2022

Rafael-Conde commented Dec 1, 2022 •

edited

Loading

djc commented Dec 2, 2022

esheppa left a comment

esheppa Dec 2, 2022

esheppa Dec 2, 2022

esheppa Dec 2, 2022

Rafael-Conde Dec 5, 2022

Rafael-Conde commented Dec 17, 2022 •

edited

Loading

pitdicker left a comment

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023 •

edited

Loading

pitdicker Jul 6, 2023 •

edited

Loading

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023

pitdicker Jul 6, 2023

djc commented Jul 6, 2023

pitdicker commented Jul 6, 2023

pitdicker commented Sep 18, 2023

Rafael-Conde commented Sep 18, 2023

pitdicker commented Sep 19, 2023

		@@ -0,0 +1,58 @@
		// I usually develop with this clippy flags
		// but I can remove once the this module is done

Years type implementation #900

Years type implementation #900

Conversation

Rafael-Conde commented Nov 26, 2022

Rafael-Conde commented Nov 26, 2022

djc commented Nov 28, 2022

Rafael-Conde commented Nov 29, 2022

djc commented Dec 1, 2022

Rafael-Conde commented Dec 1, 2022 • edited Loading

djc commented Dec 2, 2022

esheppa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rafael-Conde commented Dec 17, 2022 • edited Loading

pitdicker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitdicker Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

pitdicker Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djc commented Jul 6, 2023

pitdicker commented Jul 6, 2023

pitdicker commented Sep 18, 2023

Rafael-Conde commented Sep 18, 2023

pitdicker commented Sep 19, 2023

Rafael-Conde commented Dec 1, 2022 •

edited

Loading

Rafael-Conde commented Dec 17, 2022 •

edited

Loading

pitdicker Jul 6, 2023 •

edited

Loading

pitdicker Jul 6, 2023 •

edited

Loading