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

Support year in date formatting #7

Merged
merged 4 commits into from
Dec 12, 2023
Merged

Support year in date formatting #7

merged 4 commits into from
Dec 12, 2023

Conversation

znichollscr
Copy link
Contributor

@znichollscr znichollscr commented Dec 11, 2023

@znichollscr znichollscr marked this pull request as draft December 11, 2023 15:51
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (06b8db0) 99.41% compared to head (75b7cfe) 99.41%.

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.
📢 Have feedback on the report? Share it here.

Copy link

@mikapfl mikapfl left a 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

@znichollscr
Copy link
Contributor Author

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.

Yep this is a good point. There is further context which is why this makes sense I think:

  • the CMIP controlled vocabularies have rules for how the dates should be formatted based on the frequency (which itself follows a controlled vocabulary)
  • as a result, I want to abstract away the rules. Basically, I want a function where I can say, given this frequency rule (which could be normal things like "mon" or "yr" but also much weirder things like "3hr", "yrptC", "monC" which don't translate easily or obviously into strftime arguments), format the date.
  • So yep, right now it's super thin but this interface is actually helpful because it will soon grow to actually needing to handle weird edge cases which we will face

@mikapfl
Copy link

mikapfl commented Dec 11, 2023

I see, then the function makes sense, definitely.

@lewisjared
Copy link

Mika's question suggests the need to clarify in the docstring that ds_frequency confirms with the CMIP6 CV for frequency: https://github.com/WCRP-CMIP/CMIP6_CVs/blob/master/CMIP6_frequency.json

@znichollscr znichollscr marked this pull request as ready for review December 12, 2023 10:10
@znichollscr
Copy link
Contributor Author

Mika's question suggests the need to clarify in the docstring that ds_frequency confirms with the CMIP6 CV for frequency: https://github.com/WCRP-CMIP/CMIP6_CVs/blob/master/CMIP6_frequency.json

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

@znichollscr znichollscr merged commit ee854f0 into main Dec 12, 2023
14 checks passed
@znichollscr znichollscr deleted the yr-support branch December 12, 2023 10:11
znichollscr added a commit that referenced this pull request Dec 12, 2023
znichollscr added a commit that referenced this pull request Dec 12, 2023
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.

3 participants