-
Notifications
You must be signed in to change notification settings - Fork 13
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
adding methods for yearMonth and MonthDay #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me whenever you want to take it off draft.
The one thing missing is maybe the default implementation and some unit tests, but I don't think those are necessarily needed to merge 😄
@nekevss let me know your thoughts I've been trying to add add() but it seems it's iso is using the date version of add_date_duration, but it seems like i need an implementation more like dateTime's: Lines 125 to 164 in dd51ebb
The former doesn't use the calendar at all by the looks of it secondly why does the leap_year not return a bool? Is that wasteful? Lines 230 to 233 in dd51ebb
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some feedback regarding the most recent changes 😄
I don't think YearMonth
or MonthDay
call add or subtract directly. It's mostly done with Date
in the middle
- Fields within TemporalFields need to be visible within the crate to achieve the From<> improvement.
…us calendarId implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's one last thing that I missed the last couple times around. But outside of that this is overall looking fantastic!
- rename iso day/month - use getCalendar in leap year
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Oh, I had an unsent pending review 😅 I'll push the comment and I think we can address it on a separate PR |
// Subset of `TemporalFields` representing just the `YearMonthFields` | ||
pub struct YearMonthFields(pub i32, pub u8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to expose the fields as public?
Also, I think this should be a proper struct with named fields for clarity
Adds methods to yearMonth and monthDay