Refactor `internals` module #1212

pitdicker · 2023-08-02T10:52:36Z

As discussed in #1207.

The main motivation was removing the Of type, because it is an abstraction that is more in the way than helping.
I also moved things around to make the files smaller.
And the construction of IsoWeek and to/from Mdf is changed to not access private fields.

A lot of stuff here, but I am happy with the clean separation this brings.

pitdicker · 2023-08-02T11:31:30Z

20 commits and a green CI on first try 🎉

pitdicker · 2023-08-02T13:16:58Z

src/naive/internals.rs

-    #[inline]
-    pub(super) const fn with_ordinal(&self, ordinal: u32) -> Option<Of> {
-        let of = Of((ordinal << 4) | (self.0 & 0b1111));
-        of.validate()


Previously there was no validation of an ordinal beyond this check. Because it happened after the shift left, some invalid values where silently accepted. I added a test.

pitdicker · 2023-08-02T13:22:41Z

src/naive/internals.rs

@@ -687,33 +691,6 @@ mod tests {
        }
    }

-    #[test]
-    fn test_mdf_to_of() {


I ported a couple of tests for cases that would otherwise not be covered to naive::date::tests.
But not entirely 1:1. For example I see no added value for test_mdf_to_of when there also was test_mdf_to_of_to_mdf wich does the same thing and adds one extra step.

djc

I tried to review this but the commit history is still pretty messy IMO. I also feel like I suggested starting small in #1207 (comment) (just splitting it up). When you serve up these large PRs I have to set apart a larger chunk of (concentrated) time to try get through it, which makes it less likely that I'll be able to get to it, so I recommend splitting this into a few smaller PRs which I can more easily digest (and course correct if I think we should go in a different direction).

I think the end result is good here, I like the idea of unwinding Of and creating a cleaner separation between Mdf and NaiveDate, but to get there requires a bunch more smaller steps. If you're not already doing so, I suggest self-reviewing individual commits to check if they are easy to review (that is, don't require state that you as an author have paged in).

src/naive/date/rustc_serialize.rs

src/naive/week.rs

src/naive/date/mod.rs

src/naive/internals.rs

src/naive/isoweek.rs

src/naive/date/mod.rs

src/naive/year_flags.rs

pitdicker · 2023-08-04T06:00:32Z

Thank you for looking at it this soon! I appreciate your reviews.

I tried to review this but the commit history is still pretty messy IMO. I also feel like I suggested starting small in #1207 (comment) (just splitting it up). When you serve up these large PRs I have to set apart a larger chunk of (concentrated) time to try get through it, which makes it less likely that I'll be able to get to it, so I recommend splitting this into a few smaller PRs which I can more easily digest (and course correct if I think we should go in a different direction).

Sorry. I have spend a lot of effort in this PR to spit things up into reasonable commits, but not enough.

I don't always have an organized approach to programming. It is more like doing a couple of prototypes. Once I have something with the right direction I start picking it apart into multiple smaller commits and keep polishing them to help with review.

I'll work trough you comments an polish some more.

pitdicker · 2023-08-04T06:00:40Z

How would you like this to be split up?
First a PR to split up the files/modules, or first one to remove the Of type?

And you consider some modules too small. Before I start changing, what would be you preference for the modules to end up with?

And I think it is more useful to keep the same file structure for the naive::time, naive::date and naive::datetime modules than to have small files. Combining serde and rustc-serialize modules is also not going to do much for the 0.5 branch.

djc · 2023-08-08T09:04:03Z

I don't always have an organized approach to programming. It is more like doing a couple of prototypes. Once I have something with the right direction I start picking it apart into multiple smaller commits and keep polishing them to help with review.

I will also often end up with a larger commit than what I'd like for review, and I think that's pretty more normal. I've definitely also gone through and rebuilt a branch from scratch (maybe by cherry-picking some changes from the previous iteration). I think the main "trick" here is to re-review your changes regularly (even before committing) to see if the changes need to be together or if there is an axis along which to split.

How would you like this to be split up?
First a PR to split up the files/modules, or first one to remove the Of type?

I don't really care which one goes first, as long as it's in smaller chunks.

And you consider some modules too small. Before I start changing, what would be you preference for the modules to end up with?

My usual guideline is that 400-800 lines of code is a decent module size. This can vary a bit -- large modules are sometimes hard to prevent because that is really a natural privacy/API boundary, but I generally dislike having lots of < 200 line modules, switching between them takes more effort IMO.

And I think it is more useful to keep the same file structure for the naive::time, naive::date and naive::datetime modules than to have small files. Combining serde and rustc-serialize modules is also not going to do much for the 0.5 branch.

IMO mirroring the structure for time/date/datetime isn't so important if the relevant code size is very different. What's the issue with making merges to main harder?

pitdicker · 2023-08-11T12:40:59Z

What's the issue with making merges to main harder?

There is no issue. Just that if we combine the serde and rustc-serialize modules into one file to have a reasonable size, it will still be a small file on main which only has serde.

codecov · 2023-08-11T12:42:12Z

Codecov Report

Attention: 56 lines in your changes are missing coverage. Please review.

Comparison is base (58f1f37) 93.41% compared to head (e28f063) 91.87%.
Report is 61 commits behind head on main.

Files	Patch %	Lines
src/time_delta.rs	95.85%	31 Missing ⚠️
src/datetime/mod.rs	82.60%	8 Missing ⚠️
src/naive/datetime/mod.rs	89.33%	8 Missing ⚠️
src/naive/date.rs	98.45%	3 Missing ⚠️
src/date.rs	92.59%	2 Missing ⚠️
src/month.rs	0.00%	2 Missing ⚠️
src/offset/utc.rs	0.00%	1 Missing ⚠️
src/weekday.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1212      +/-   ##
==========================================
- Coverage   93.41%   91.87%   -1.55%     
==========================================
  Files          34       38       +4     
  Lines       16754    17507     +753     
==========================================
+ Hits        15651    16084     +433     
- Misses       1103     1423     +320

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pitdicker · 2023-08-11T12:43:36Z

I have spend some time in git rebase for this PR. It now only has the changes to remove the Of type and DateImpl.

I did not find a really clean way to split the last three commits.

pitdicker · 2023-08-11T12:45:05Z

Hmm. Is a -0.05% decrease in code coverage a reason to fail a CI run?

djc · 2023-08-11T13:03:59Z

Hmm. Is a -0.05% decrease in code coverage a reason to fail a CI run?

I will not hesitate a PR like that even if CI is failing. If you want to adjust the CodeCov config for it, go ahead.

remove `Of::isoweekdate_raw`

pitdicker

I have gone over all commits again (it was all pretty paged out). I renamed one commit, but did not see something that felt like it should change.

The commits are ordered so we can move one method at a time to NaiveDate. Which was tricky because of all the interdependencies.
The moved code usually does not look the same. I find it is best to read the new code with the bit representation in mind to see why it makes sense.

The more a commit gets to the end op this PR, the more helper methods or now useless tests open up and can be removed with it.

djc · 2024-02-12T09:24:10Z

Okay, can you split this up into 3-4 PRs somehow and do one at a time?

pitdicker · 2024-02-12T09:41:40Z

Sure, #1428 has the first four.

djc · 2024-02-12T13:39:04Z

Thanks for accommodating me!

pitdicker · 2024-02-12T13:49:15Z

And I want to thank you a lot for all the review work today!

pitdicker mentioned this pull request Aug 2, 2023

Use NonZeroI32 inside NaiveDate #1207

Merged

pitdicker commented Aug 2, 2023

View reviewed changes

pitdicker force-pushed the merge_internals branch from d603a26 to 36fd78e Compare August 2, 2023 13:26

djc reviewed Aug 2, 2023

View reviewed changes

pitdicker force-pushed the merge_internals branch from 36fd78e to b2e6ef6 Compare August 11, 2023 12:39

pitdicker mentioned this pull request Aug 11, 2023

Add .github/codecov.yml and set threshold to 1% #1215

Merged

pitdicker force-pushed the merge_internals branch from 60d2eb0 to 371a906 Compare August 12, 2023 09:05

pitdicker force-pushed the merge_internals branch from 371a906 to 332c19a Compare September 27, 2023 12:42

pitdicker mentioned this pull request Sep 27, 2023

0.5 release planning #970

Open

pitdicker force-pushed the merge_internals branch from 332c19a to 1a93af0 Compare September 29, 2023 09:30

pitdicker force-pushed the merge_internals branch from 1a93af0 to d5bafd6 Compare January 26, 2024 20:49

This was referenced Jan 30, 2024

Add optional borsh serialization/deserialization #1366

Closed

0.5: Converting the API to return Results (part 1) #1410

Merged

pitdicker mentioned this pull request Feb 7, 2024

Recognize our review capacity is limited for non-trivial work #1417

Open

pitdicker added 6 commits February 7, 2024 17:16

Remove DateImpl type alias

150a643

Rename ymdf field to yof

630f01f

Add description of NaiveDate internal format

16ad3f3

Move MIN_YEAR and MAX_YEAR constants

5443d7a

Remove Of::succ

55c47af

Remove Of::pred

8653930

Remove Of::with_ordinal

f4f3018

pitdicker force-pushed the merge_internals branch from d5bafd6 to 9960a41 Compare February 7, 2024 16:22

pitdicker added 2 commits February 11, 2024 08:39

Remove Of::ordinal

253a77c

Move iso_week_from_yof to IsoWeek::from_yof

41e083f

pitdicker force-pushed the merge_internals branch from 9960a41 to 4b03ebd Compare February 11, 2024 07:39

pitdicker added 8 commits February 11, 2024 15:01

Change IsoWeek::from_yof to consuming ordinal and year_flags

6e09bff

remove `Of::isoweekdate_raw`

Remove Of::weekday

d3b6b6e

Remove Of::new

016e975

Remove Of::flags

7f8a355

Change Mdf::from_of to Mdf::from_ol

50185eb

Add Mdf::ordinal and Mdf::year_flags

c25bdf7

Remove NaiveDate::with_of

562d7fd

Remove Mdf::to_of

e28f063

pitdicker force-pushed the merge_internals branch from 4b03ebd to e28f063 Compare February 11, 2024 14:06

pitdicker commented Feb 11, 2024

View reviewed changes

pitdicker mentioned this pull request Feb 12, 2024

Refactor internals module part 1 #1428

Merged

pitdicker marked this pull request as draft February 12, 2024 11:20

pitdicker mentioned this pull request Feb 12, 2024

Refactor internals module part 5 #1433

Merged

pitdicker closed this Feb 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `internals` module #1212

Refactor `internals` module #1212

pitdicker commented Aug 2, 2023

pitdicker commented Aug 2, 2023

pitdicker Aug 2, 2023

pitdicker Aug 2, 2023

djc left a comment

pitdicker commented Aug 4, 2023

pitdicker commented Aug 4, 2023

djc commented Aug 8, 2023

pitdicker commented Aug 11, 2023

codecov bot commented Aug 11, 2023 •

edited

Loading

pitdicker commented Aug 11, 2023

pitdicker commented Aug 11, 2023

djc commented Aug 11, 2023 •

edited

Loading

pitdicker left a comment

djc commented Feb 12, 2024

pitdicker commented Feb 12, 2024

djc commented Feb 12, 2024

pitdicker commented Feb 12, 2024

Refactor internals module #1212

Refactor internals module #1212

Conversation

pitdicker commented Aug 2, 2023

pitdicker commented Aug 2, 2023

pitdicker Aug 2, 2023

Choose a reason for hiding this comment

pitdicker Aug 2, 2023

Choose a reason for hiding this comment

djc left a comment

Choose a reason for hiding this comment

pitdicker commented Aug 4, 2023

pitdicker commented Aug 4, 2023

djc commented Aug 8, 2023

pitdicker commented Aug 11, 2023

codecov bot commented Aug 11, 2023 • edited Loading

Codecov Report

pitdicker commented Aug 11, 2023

pitdicker commented Aug 11, 2023

djc commented Aug 11, 2023 • edited Loading

pitdicker left a comment

Choose a reason for hiding this comment

djc commented Feb 12, 2024

pitdicker commented Feb 12, 2024

djc commented Feb 12, 2024

pitdicker commented Feb 12, 2024

Refactor `internals` module #1212

Refactor `internals` module #1212

codecov bot commented Aug 11, 2023 •

edited

Loading

djc commented Aug 11, 2023 •

edited

Loading