-
Notifications
You must be signed in to change notification settings - Fork 94
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
Duration filters #2682
Duration filters #2682
Conversation
Map an ISO8601 duration to a floating-point number of seconds for use in suite definition files that require a duration in non-ISO8601 units.
Map an ISO8601 duration to a floating-point number of hours for use in suite definition files that require a duration in non-ISO8601 format.
This builds both the PDF and HTML documentation, but has some funny effects in the HTML code where the underscores display properly in the PDF. Not sure how to address this.
I may need some assistance from the more LateX-savvy developers. Getting the underscore in the paragraph labels (to be consistent with the other filter documentation) works for PDF, but resulted (at least for me) in some weird superscript characters or something in the HTML. |
The LaTeX to HTML conversion is generally awful (bring on Sphinx...) but I'll take a quick look soon... |
(@trwhitcomb - sorry for the delay on this, we have a lot on at the moment, will get to it soon...) |
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, and I can't see any spurious characters in the HTML user guide. Can you just extend tests/jinja2/09-custom-jinja2-filters.t
to cover these new filters though?
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.
This looks fine to me. My only comment is it would be nice to combine these as I can foresee the requirement for duration_to_days
etc.
>>> duration('PT1H', '%S')
3600
>>> duration('PT1H', '%H')
1
{{ 'PT1H' | duration('%S') }}
# 3600
{{ 'PT1H' | duration('%H') }}
# 1
I second @oliver-sanders' idea. |
@hjoliver @oliver-sanders This is definitely where I wanted to go, and I agree that it might be nice to combine them, and I originally started down that road. However, I wrapped myself around the axle for what the particular formats should be and, more importantly, what they should mean. When talking about time point formats, it's clear that something like "%s" is the seconds component of the date. When we're talking about durations, it's ambiguous - you could be asking for "%s" to mean the seconds component of a duration (so for 'PT1H' it is 0) or you could mean the total seconds contained within a duration (i.e. 'PT1H' would be 3600.0). I made the decision to keep these separate so reading the template would be more explicit, but also to highlight that it's the total time of the duration that happens to be expressed in those units (also why it's not an integer). let me know what you think, but that's the direction that I was going. |
Agreed the '%H' format I posted above is a bad idea, it implies you could also do '%H%S' etc, it was more for a gauge of the idea. How about an explicit filter performing the functionality of both, some options:
|
I agree with @oliver-sanders in that a single multi-purpose filter could be extended in the future rather than introducing more filters. (Climate suites might want duration as months or years...). And yeah, normal template notation |
I like the keyword syntax the most: A side note, with #2733 in master, you can do that without any filter now 😁
|
@TomekTrzeciak raises a good point - post #2733 do we still need this PR? We could just document direct use of the isodatetime Python library... |
Possibly, though the interface of the |
@hjoliver @TomekTrzeciak #2733 looks really nice (I was just bemoaning the lack of "zip"!), but I think this should remain separate - rather than having the import in every single routine it's nice to have a few "builtin" convenience routines, especially once I get the keyword syntax implemented in there. Sort of similar to how the update in #2733 would obviate the existing |
@trwhitcomb - agreed, let's keep this PR. We will await your change to the interface as suggested above... (note also there's a minor conflict in the User Guide now). |
Ping @trwhitcomb - I'm bumping this back to "soon" as we need to get next release out by end of month. (We can restore it to next-release if it turns out you are able to finish it off in time). |
Use a switchable unit rather than separate functions for each unit for the duration conversion. Stick with everything up to weeks for now as after that (e.g. months, years) things become ambiguous with multiple days per month and multiple days per year.
Make requested changes from PR to CUG source, update CUG source to account for filter consolidation, and add tests as requested by the PR.
@hjoliver thanks for the reminder! Updates attached. |
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.
@trwhitcomb - you have a very minor conflict in the user guide. And one codacy-prompted improvement.
Apparently codacy prefers succinctness to symmetry, so remove a lambda that really doesn't need to be a lambda in order to remove the automated review flag.
@hjoliver thanks - I think I got them both. |
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!
pycodestyle
would like some whitespace changed (https://travis-ci.org/cylc/cylc/jobs/446433022#L1224), otherwise good to go.
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.
OK, we are good here.
The Cylc Contributor Licence Agreement has changed since your last commit, before we can merge you need to agree to the new CLA by removing the parenthesis around your name in CONTRIBUTING.md
.
See https://github.com/cylc/cylc/blob/master/CONTRIBUTING.md#code-contributors for details.
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.
Thanks @trwhitcomb!
Add 2 new built-in Jinja2 filters that are able to take ISO8601 durations and convert them (using the builtin isodatetime module) into either decimal seconds or decimal hours. These allow ISO8601 durations to be specified in the suite, then used for tasks that aren't able to process ISO8601 syntax themselves.
By default, they return floating-point numbers in case the duration cannot be expressed directly as either seconds (unlikely) or hours (definitely possible). If an integer is desired, chain the Jinja2
int
filter.