Add duration formatter #2008
Add duration formatter #2008
Conversation
|
It is hard to judge/review until there is at least one block using this. The good thing is that blocks can be migrated in a non-breaking way, for example By the way, I think it is safe to delete |
|
Added two examples, uptime and tea_timer. Still need to doc the formatter... |
|
Do I understand correctly that currently there is no way to use By the way, maybe |
That's right. If you wanted to do [hh]:mm:ss you'd want to do something like The most flexible solution would be to allow an explicit format to be specified if we want to allow "sub-formats" like: or using the allow argument mentioned in #1957:
I set |
|
Another option would be to use strftime style formats. |
|
Perhaps adding an |
That's... too much formatting 😄
Again, nested format strings feel weird. But maybe... maybe we can add support for some sort of "fields", like |
I'm not sure that the field concept would be generally useful (outside of the duration format) and much more verbose so I'm leaning towards the |
|
I think the behaviour of |
|
I guess we should decide if hms should be enabled by default for blocks that used that format previously or if it is better to use the unambiguous default format. (2 units of hms are either hh:mm or mm::ss, but unless you know the interval/or it's a small interval and you can see the seconds changing) |
|
Once we think the settings/defaults make sense I'll add some unit tests as well. |
|
@MaxVerevkin I know this is a lot to review, do you want me to move the splitting up of the formatters into modules into its own PR? |
That would be nice :) |
|
@MaxVerevkin, I rebased on master now that the formatter split and allow empty strings PRs have merged. |
Yes, let's try to keep the defaults as they are. I'll try to take a closer look tomorrow. |
|
The types section of the docs needs to be updated. |
|
@bim9262 Is this ready to be merged? |
|
I still wanted to add some tests |
|
@MaxVerevkin I've added some tests :) |
There was a problem hiding this comment.
This currently changes the way $time is rendered in battery block, because hms is not the default and the behavior is preserved only if explicit .str() is used. I think there are two things we can do:
- Make
Value::Durationhold the default value forhms. - Do not touch
timeand instead deprecate it and addremaining/duration/something.
src/formatting/formatter/duration.rs
Outdated
| macro_rules! fmt { | ||
| ($name:ident) => {{ | ||
| fmt!($name,) | ||
| }}; | ||
| ($name:ident, $($key:ident : $value:tt),* $(,)?) => { | ||
| new_formatter(stringify!($name), &[ | ||
| $( Arg { key: stringify!($key), val: stringify!($value) } ),* | ||
| ]) | ||
| }; | ||
| } |
There was a problem hiding this comment.
eng.rs has a similar macro for tests, let's share them?
There was a problem hiding this comment.
Created #2054, will rebase after that's merged
Can you explain what you mean? Hold the string value like I did with
Got it, I can add another value instead changing the type of time. |
Something like |
The format for
I had thought about doing something like: #[derive(Debug, Default)]
pub struct DurationFormatter(Option<DurationFormatterInner>);
#[derive(Debug, Default)]
pub struct DurationFormatterInner {
hms: bool,
max_unit_index: usize,
min_unit_index: usize,
units: usize,
round_up: bool,
unit_has_space: bool,
pad_with: PadWith,
show_leading_units_if_zero: bool,
}but I dislike having different defaults for different blocks. So I think I'll follow the approach:
|
|
@MaxVerevkin anything else needed here or are we good to merge? |
|
@MaxVerevkin, I made the suggested changes |
No worries, I've also been late on the reviews too... |
TODO: document duration formatter.
Any thoughts on the names of the arguments, the format, or the defaults?
The formatted can be used by the battery, uptime, tea_timer and pomodoro blocks.