Skip to content
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

Refactor internals module #1212

Closed
wants to merge 17 commits into from
Closed

Conversation

pitdicker
Copy link
Collaborator

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
Copy link
Collaborator Author

20 commits and a green CI on first try 🎉

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

#[test]
fn test_mdf_to_of() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src/naive/week.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/internals.rs Show resolved Hide resolved
src/naive/isoweek.rs Show resolved Hide resolved
src/naive/date/mod.rs Outdated Show resolved Hide resolved
src/naive/year_flags.rs Outdated Show resolved Hide resolved
@pitdicker
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Member

djc commented Aug 8, 2023

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
Copy link
Collaborator Author

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
Copy link

codecov bot commented Aug 11, 2023

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@djc
Copy link
Member

djc commented Aug 11, 2023

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.

Copy link
Collaborator Author

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

djc commented Feb 12, 2024

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

@pitdicker
Copy link
Collaborator Author

Sure, #1428 has the first four.

@pitdicker pitdicker marked this pull request as draft February 12, 2024 11:20
@pitdicker pitdicker closed this Feb 12, 2024
@djc
Copy link
Member

djc commented Feb 12, 2024

Thanks for accommodating me!

@pitdicker
Copy link
Collaborator Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants