Refactor internals module part 5
#1433
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1433 +/- ##
==========================================
- Coverage 91.87% 91.84% -0.03%
==========================================
Files 38 40 +2
Lines 17507 17450 -57
==========================================
- Hits 16084 16027 -57
Misses 1423 1423 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
So I think we discussed this before, but still not a big fan of the module split as proposed in this PR. All of serde, rustc_serialize and week end up being too small to be worthy of a separate file, IMO. I would suggest moving everything that's not an impl NaiveDate or impl .. for NaiveDate into a types module, maybe?
| @@ -1427,7 +1432,7 @@ impl NaiveDate { | |||
| /// specified. | |||
| #[inline] | |||
| pub const fn week(&self, start: Weekday) -> NaiveWeek { | |||
| NaiveWeek { date: *self, start } | |||
| NaiveWeek::new(*self, start) | |||
There was a problem hiding this comment.
Just to make sure: we are looking at NaiveWeek::new and NaiveDate::week. I don't think there is much I can reorder? Or I don't understand your comment.
There was a problem hiding this comment.
If week() uses new(), the former should be defined before the latter.
There was a problem hiding this comment.
Okay, I have been using that principle lately.
There was a problem hiding this comment.
It's still not looking the way I'd expect it to?
There was a problem hiding this comment.
It is not part of the same impl block or the same type.
src/naive/date/serde.rs
Outdated
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use crate::naive::date::{test_decodable_json, test_encodable_json}; |
There was a problem hiding this comment.
I think test_decodable_json and test_encodable_json should move into tests.
There was a problem hiding this comment.
Is it okay if I do that as a separate PR for these functions in all four modules (including naive::time, naive::datetime and datetime?
|
Yes, the And move |
Yes, those sound like nice improvements. |
pitdicker
force-pushed
the
refactor_internals_5
branch
from
7834d6d
to
bf92d4d
Compare
These are the simple commits that followed after #1212 but where split out.
naive::dateinto separate files to get the same folder structure asnaive::timeandnaive::datetime.NaiveWeektype to its own module. It needs an extranewmethod because its fields are now private.