[snapshots] use user locale in timestamp output #3237
Conversation
| std::time_t t = google::protobuf::util::TimeUtil::TimestampToTimeT(timestamp); | ||
|
|
||
| // ISO 8601: YYYY-MM-DDThh:mm:ss±hhmm | ||
| oss << std::put_time(std::localtime(&t), "%FT%T%z"); |
There was a problem hiding this comment.
Currently have this set up to print out the UTC offset. Excluding ±hhmm looks a bit more presentable, but then means that the datetime is ambiguous on whether it defines UTC time or localtime. See ISO 8601.
There was a problem hiding this comment.
This converts to localtime, which is also necessary (I thought was already the case, so good thing you did that). However, you use a format that is not influenced by the locale (it is standard). To use the locale, I think you would need something like "%c %Z".
The locale needs to be set before this will work, and finding the current locale is platform dependent (e.g. LC_* environment vars on Linux). However, I believe Qt does that for its CoreApplication, so hopefully that would already work. It wouldn't hurt to test on Windows too.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3237 +/- ##
==========================================
- Coverage 84.03% 84.00% -0.04%
==========================================
Files 250 250
Lines 13464 13482 +18
==========================================
+ Hits 11315 11326 +11
- Misses 2149 2156 +7 ☔ View full report in Codecov by Sentry. |
|
Thanks for this @sharder996. I will enqueue this with a little lower priority to focus on PRs that were already on flight. But hopefully we can still get it into the MVP. |
ricab
force-pushed
the
cli-use-local-time
branch
2 times, most recently
from
ce8804a to
9125f5f
Compare
ricab
force-pushed
the
snapshots
branch
2 times, most recently
from
ef4a85e to
8b3c7a6
Compare
ricab
force-pushed
the
cli-use-local-time
branch
from
9125f5f to
09189a0
Compare
|
Hey @sharder, could you please rebase the relevant commits here? Thank you! |
sharder996
force-pushed
the
cli-use-local-time
branch
from
09189a0 to
755169a
Compare
There was a problem hiding this comment.
Hey @sharder996, thanks for this PR! It looks good overall, both structurally and in detail. However, you are using local time, but not a locale yet. The locale is important in this instance mainly to avoid ambiguity in the date (is 2022-11-02 the same as Nov 2nd or Feb 11th?); to a lesser extent, in the time too (is 1:15 am or pm?).
In any case, this is certainly valuable as is. If it turns out to be difficult to use the locale, we can skip that and accept. But please have a look at my inline comment.
| std::time_t t = google::protobuf::util::TimeUtil::TimestampToTimeT(timestamp); | ||
|
|
||
| // ISO 8601: YYYY-MM-DDThh:mm:ss±hhmm | ||
| oss << std::put_time(std::localtime(&t), "%FT%T%z"); |
There was a problem hiding this comment.
This converts to localtime, which is also necessary (I thought was already the case, so good thing you did that). However, you use a format that is not influenced by the locale (it is standard). To use the locale, I think you would need something like "%c %Z".
The locale needs to be set before this will work, and finding the current locale is platform dependent (e.g. LC_* environment vars on Linux). However, I believe Qt does that for its CoreApplication, so hopefully that would already work. It wouldn't hurt to test on Windows too.
There was a problem hiding this comment.
Thanks @sharder996! This doesn't work within a snap, but I think we can get it in and deal with that separately. I have only a tiny request inline, then we can merge.
| #include <multipass/cli/json_formatter.h> | ||
| #include <multipass/cli/table_formatter.h> | ||
| #include <multipass/cli/yaml_formatter.h> | ||
|
|
||
| #include <sstream> | ||
|
|
|
bors r+ |
|
Oh oh is bors gone for good? |
|
bors ping |
|
Ok, CI is all green as are integration tests. Merging... |
No description provided.