-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add Accessors for Day and higher #478
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #478 +/- ##
==========================================
- Coverage 92.79% 92.57% -0.23%
==========================================
Files 39 38 -1
Lines 1818 1831 +13
==========================================
+ Hits 1687 1695 +8
- Misses 131 136 +5 ☔ View full report in Codecov by Sentry. |
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.
Can you add tests?
I originally added this accessor code when Dates.jl narrowed the types used for these sub-day accessors: 61d8299. |
@omus I've added a block of tests to use the |
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.
Better to not access the internal fields
Co-authored-by: Curtis Vogt <curtis.vogt@gmail.com>
In looking over the code again I realized that part of the original implementation was avoiding defining additional methods where they weren't required. Defining methods for |
Looks like I'll be dealing with LTS issues before getting this in. Update: CI WeakDeps failure was just a fluke |
During a development project, I noticed that I was not able to run
Day
(or higher) onZonedDateTime
objects. The fix in this PR addresses that issue. Below is a minimum working example to show the issue