-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
[feature][expressions] Add format_interval function #55172
Conversation
cf23c6a
to
f25f749
Compare
Updated to fix code formatting issue and rebase on master |
f25f749
to
0054ccc
Compare
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 @Malvineous
Trying to provide a feedback, not on code side but on the expression help content.
I personally find it quite verbose and wondered if some details (somehow technical and describing internal logic) should not be showcased in examples instead of text. Tried below.
Also how do you decide whether the letters should be uppercase (Y,M,W,D) or lowercase (h,m,s,z)? Time vs date, I guess. How about distinguish only month vs minute? (I think this is the usage but I may be wrong, i don't have much experience)
"expression": "format_interval(make_interval(minutes:=1560),'%Dd %mm')", | ||
"returns": "'1d 120m'" | ||
}], | ||
"tags": ["custom", "type", "uses", "format", "strings", "time", "date", "formats", "see", "interval", "tostring"] |
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.
"tags": ["custom", "type", "uses", "format", "strings", "time", "date", "formats", "see", "interval", "tostring"] | |
"tags": ["custom", "type", "format", "strings", "time", "date", "formats", "conversion", "interval", "tostring"] |
I do not find them relevant here. Not also convinced by the tostring...
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.
I wanted to provide plenty of information and examples to make things clear, but if you think could be better explained I'm happy to make changes. I was hoping by providing a few examples users could skip reading the text and just look at the examples to figure it out if they preferred.
For the tags I just copied them off the format_date
function as this function does something very similar, so do you think your proposed changes here should also apply to format_date
?
"description": "interval value, such as that returned by make_interval()." | ||
}, { | ||
"arg": "format", | ||
"description": "String template used to format the string. <table><thead><tr><th>Expression</th><th>Output</th></tr></thead><tr valign=\"top\"><td><code>%Y</code></td><td>the number of years</td></tr><tr valign=\"top\"><td><code>%M</code></td><td>the number of months</td></tr><tr valign=\"top\"><td><code>%W</code></td><td>the number of weeks</td></tr><tr valign=\"top\"><td>%D</td><td>the number of days</td></tr><tr valign=\"top\"><td>%h</td><td>the number of hours</td></tr><tr valign=\"top\"><td>%m</td><td>the number of minutes</td></tr><tr valign=\"top\"><td>%s</td><td>the number of seconds</td></tr><tr valign=\"top\"><td>%z</td><td>the number of milliseconds</td></tr></table>A single zero can be inserted before the specifier (e.g. <code>%0m</code>) to add a leading zero if the value would be less than two digits. Milliseconds can be padded to three digits (<code>%00z</code>) and years up to four digits (<code>%000Y</code>). This is useful for formats like <code>%h:%0m:%0s.%00z</code> to display as <code>2:04:06.008</code> rather than <code>2:4:6.8</code>.<br/>The interval is only broken down to the largest specifier. So for an interval of 65 seconds, only using the <code>%s</code> specifier will show 65 as the value for the seconds field, however if the <code>%m</code> specifier is also used, the value will be broken down into minutes instead, so the same interval will show as 1 minute 5 seconds. This means you do not have to specify all placeholders - if you omit <code>%h</code> for hours, the result is that <code>%m</code> for minutes will increase above 59 if needed.<br/>The least significant placeholder specified (with the year being the most significant and milliseconds the least) will be rounded up or down as appropriate. This way an interval of 90 seconds will show as 2 minutes if only <code>%m</code> is used, or 1 minute 30 seconds if both <code>%m</code> and <code>%s</code> are used together." |
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.
I wonder if examples should not showcase what you are describing here
"description": "String template used to format the string. <table><thead><tr><th>Expression</th><th>Output</th></tr></thead><tr valign=\"top\"><td><code>%Y</code></td><td>the number of years</td></tr><tr valign=\"top\"><td><code>%M</code></td><td>the number of months</td></tr><tr valign=\"top\"><td><code>%W</code></td><td>the number of weeks</td></tr><tr valign=\"top\"><td>%D</td><td>the number of days</td></tr><tr valign=\"top\"><td>%h</td><td>the number of hours</td></tr><tr valign=\"top\"><td>%m</td><td>the number of minutes</td></tr><tr valign=\"top\"><td>%s</td><td>the number of seconds</td></tr><tr valign=\"top\"><td>%z</td><td>the number of milliseconds</td></tr></table>A single zero can be inserted before the specifier (e.g. <code>%0m</code>) to add a leading zero if the value would be less than two digits. Milliseconds can be padded to three digits (<code>%00z</code>) and years up to four digits (<code>%000Y</code>). This is useful for formats like <code>%h:%0m:%0s.%00z</code> to display as <code>2:04:06.008</code> rather than <code>2:4:6.8</code>.<br/>The interval is only broken down to the largest specifier. So for an interval of 65 seconds, only using the <code>%s</code> specifier will show 65 as the value for the seconds field, however if the <code>%m</code> specifier is also used, the value will be broken down into minutes instead, so the same interval will show as 1 minute 5 seconds. This means you do not have to specify all placeholders - if you omit <code>%h</code> for hours, the result is that <code>%m</code> for minutes will increase above 59 if needed.<br/>The least significant placeholder specified (with the year being the most significant and milliseconds the least) will be rounded up or down as appropriate. This way an interval of 90 seconds will show as 2 minutes if only <code>%m</code> is used, or 1 minute 30 seconds if both <code>%m</code> and <code>%s</code> are used together." | |
"description": "String template used to format the string. <table><thead><tr><th>Expression</th><th>Output</th></tr></thead><tr valign=\"top\"><td><code>%Y</code></td><td>the number of years</td></tr><tr valign=\"top\"><td><code>%M</code></td><td>the number of months</td></tr><tr valign=\"top\"><td><code>%W</code></td><td>the number of weeks</td></tr><tr valign=\"top\"><td>%D</td><td>the number of days</td></tr><tr valign=\"top\"><td>%h</td><td>the number of hours</td></tr><tr valign=\"top\"><td>%m</td><td>the number of minutes</td></tr><tr valign=\"top\"><td>%s</td><td>the number of seconds</td></tr><tr valign=\"top\"><td>%z</td><td>the number of milliseconds</td></tr></table>Not all placeholders need to be specified; the interval is broken down to only the specified ones and will be rounded up or down as appropriate.<br/>Zeros can be inserted before the specifier (e.g. <code>%0m</code>) to add a leading zero if the returned value would be less than a desired number of digits. This is useful for formats like <code>%h:%0m:%0s.%00z</code> to display as <code>2:04:06.008</code> rather than <code>2:4:6.8</code>." |
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.
Happy to remove some of the explanation. Having worked in IT support for many years I am used to spending a lot of time explaining things to people so I tend to include a lot of detail, to avoid people coming back and asking more questions later :)
}, { | ||
"expression": "format_interval(make_interval(minutes:=91),'%h hr')", | ||
"returns": "'2 hr'" | ||
}, { |
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.
Assuming I understand the description.
}, { | |
}, { | |
"expression": "format_interval(make_interval(minutes:=62), '%h hr')", | |
"returns": "'1 hr'" | |
}, { | |
"expression": "format_interval(make_interval(minutes:=62), '%h hr %s')", | |
"returns": "'1 hr 120s'" | |
}, { |
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.
Yes that's correct, only the second format specifier must be %ss
to show 120s
(as written it will show 1 hr 120
). Happy to add these examples although for uniformity I think it looks better using either hr
and sec
or h
and s
but not mixing both variants.
Forgot to say: thanks for providing a new feature and for your patience. |
@DelazJ No problems at all, many thanks for reviewing! |
@Malvineous, thanks for submitting this PR. |
Thanks @agiudiceandrea let's hope so! |
const QgsInterval interval = QgsExpressionUtils::getInterval( values.at( 0 ), parent, true ); | ||
QString format = QgsExpressionUtils::getStringValue( values.at( 1 ), parent ); |
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.
const QgsInterval interval = QgsExpressionUtils::getInterval( values.at( 0 ), parent, true ); | |
QString format = QgsExpressionUtils::getStringValue( values.at( 1 ), parent ); | |
const QgsInterval interval = QgsExpressionUtils::getInterval( values.at( 0 ), parent, true ); | |
if ( !interval.isValid() ) | |
return QVariant(); | |
QString format = QgsExpressionUtils::getStringValue( values.at( 1 ), parent ); |
bool has_Y = format.contains( "%Y" ) || format.contains( "%0Y" ) | ||
|| format.contains( "%00Y" ) || format.contains( "%000Y" ); | ||
bool has_M = format.contains( "%M" ) || format.contains( "%0M" ); | ||
bool has_W = format.contains( "%W" ) || format.contains( "%0W" ); | ||
bool has_D = format.contains( "%D" ) || format.contains( "%0D" ); | ||
bool has_h = format.contains( "%h" ) || format.contains( "%0h" ); | ||
bool has_m = format.contains( "%m" ) || format.contains( "%0m" ); | ||
bool has_s = format.contains( "%s" ) || format.contains( "%0s" ); | ||
bool has_z = format.contains( "%z" ) || format.contains( "%0z" ) | ||
|| format.contains( "%00z" ); |
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.
bool has_Y = format.contains( "%Y" ) || format.contains( "%0Y" ) | |
|| format.contains( "%00Y" ) || format.contains( "%000Y" ); | |
bool has_M = format.contains( "%M" ) || format.contains( "%0M" ); | |
bool has_W = format.contains( "%W" ) || format.contains( "%0W" ); | |
bool has_D = format.contains( "%D" ) || format.contains( "%0D" ); | |
bool has_h = format.contains( "%h" ) || format.contains( "%0h" ); | |
bool has_m = format.contains( "%m" ) || format.contains( "%0m" ); | |
bool has_s = format.contains( "%s" ) || format.contains( "%0s" ); | |
bool has_z = format.contains( "%z" ) || format.contains( "%0z" ) | |
|| format.contains( "%00z" ); | |
const bool has_Y = format.contains( "%Y" ) || format.contains( "%0Y" ) | |
|| format.contains( "%00Y" ) || format.contains( "%000Y" ); | |
const bool has_M = format.contains( "%M" ) || format.contains( "%0M" ); | |
const bool has_W = format.contains( "%W" ) || format.contains( "%0W" ); | |
const bool has_D = format.contains( "%D" ) || format.contains( "%0D" ); | |
const bool has_h = format.contains( "%h" ) || format.contains( "%0h" ); | |
const bool has_m = format.contains( "%m" ) || format.contains( "%0m" ); | |
const bool has_s = format.contains( "%s" ) || format.contains( "%0s" ); | |
const bool has_z = format.contains( "%z" ) || format.contains( "%0z" ) | |
|| format.contains( "%00z" ); |
const qlonglong ms_s = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Seconds, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_m = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Minutes, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_h = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Hours, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_D = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Days, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_W = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Weeks, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_M = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Months, Qgis::TemporalUnit::Milliseconds ); | ||
const qlonglong ms_Y = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Years, Qgis::TemporalUnit::Milliseconds ); |
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.
These are all static constants, so we can optimise this a bit:
const qlonglong ms_s = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Seconds, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_m = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Minutes, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_h = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Hours, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_D = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Days, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_W = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Weeks, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_M = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Months, Qgis::TemporalUnit::Milliseconds ); | |
const qlonglong ms_Y = QgsUnitTypes::fromUnitToUnitFactor( Qgis::TemporalUnit::Years, Qgis::TemporalUnit::Milliseconds ); | |
constexpr qlonglong ms_s = 1000; | |
constexpr qlonglong ms_m = 60000; | |
constexpr qlonglong ms_h = 3600000; | |
constexpr qlonglong ms_D = 86400000; | |
constexpr qlonglong ms_W = 604800000; | |
constexpr qlonglong ms_M = 2592000000; | |
constexpr qlonglong ms_Y = 31557600000; |
qlonglong value; | ||
if ( isLeastSignificant_Y ) | ||
{ | ||
value = std::round( double( outstanding ) / ms_Y ); |
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.
value = std::round( double( outstanding ) / ms_Y ); | |
value = std::round( static_cast< double >( outstanding ) / ms_Y ); |
QString strValue; | ||
strValue.setNum( value ); |
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.
QString strValue; | |
strValue.setNum( value ); | |
const QString strValue = QString::number( value ); |
qlonglong value; | ||
if ( isLeastSignificant_m ) | ||
{ | ||
value = std::round( double( outstanding ) / ms_m ); |
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.
value = std::round( double( outstanding ) / ms_m ); | |
value = std::round( static_cast< double >( outstanding ) / ms_m ); |
QString strValue; | ||
strValue.setNum( value ); |
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.
QString strValue; | |
strValue.setNum( value ); | |
const QString strValue = QString::number( value ); |
qlonglong value; | ||
if ( isLeastSignificant_s ) | ||
{ | ||
value = std::round( double( outstanding ) / ms_s ); |
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.
value = std::round( double( outstanding ) / ms_s ); | |
value = std::round( static_cast< double >( outstanding ) / ms_s ); |
QString strValue; | ||
strValue.setNum( value ); |
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.
QString strValue; | |
strValue.setNum( value ); | |
const QString strValue = QString::number( value ); |
QString strValue; | ||
strValue.setNum( value ); |
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.
QString strValue; | |
strValue.setNum( value ); | |
const QString strValue = QString::number( value ); |
@Malvineous I've made some minor style recommendations above. Nothing major, just little cleanups. One thing I'd like to see here is either:
If the documentation issue is addressed then I'll approve and merge! (And thanks for your patience here!) |
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
Thanks @nyalldawson for the feedback and code review! I will make those changes soon. |
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
Haven't forgotten about this, just need to find the time to address the feedback. Commenting to keep the issue open. |
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
While we hate to see this happen, this PR has been automatically closed because it has not had any activity in the last 21 days. If this pull request should be reconsidered, please follow the guidelines in the previous comment and reopen this pull request. Or, if you have any further questions, just ask! We love to help, and if there's anything the QGIS project can do to help push this PR forward please let us know how we can assist. |
Description
This adds a new
format_interval
function to address feature request #54257. It takes an interval value and a format specifier, and returns a string describing the interval in terms of the format specifier (e.g.5 years, 2 months
or3:01
).I have included some tests and they pass with
make test
.Here is a screenshot of the help text with some examples: