-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support year in date formatting #7
Conversation
znichollscr
commented
Dec 11, 2023
•
edited
Loading
edited
- Update copier #8
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 11 11
Lines 342 343 +1
Branches 47 48 +1
=======================================
+ Hits 340 341 +1
Partials 2 2 ☔ 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.
Hi,
I think in the context of what's already there, this is fine, an improvement over the status quo.
However, I don't like the function itself. Why have a format_date
function if it is a super thin wrapper around strftime
? Maybe I'm very unix-damaged, but for me date.strftime("%Y")
immediately shows what is happening, while format_date(date, "yr")
means I have to click through to read the body of the function to know what's happening. So, I'd advocate for deleting the format_date
function instead unless there is another reason to keep it that I missed.
Cheers,
Mika
Yep this is a good point. There is further context which is why this makes sense I think:
|
I see, then the function makes sense, definitely. |
Mika's question suggests the need to clarify in the docstring that |
Yes, but it's actually not that CV, but this one https://github.com/PCMDI/input4MIPs-cmor-tables/blob/master/input4MIPs_frequency.json I will speak to Paul about this and then propagate back. I've made #10 as a reminder |
CHANGELOG that was forgotten in #7