Skip to content

Add display hint to output u64 as ISO8601 time#617

Merged
bors[bot] merged 4 commits intomainfrom
feature/display-hint-unix-timestamp
Oct 27, 2021
Merged

Add display hint to output u64 as ISO8601 time#617
bors[bot] merged 4 commits intomainfrom
feature/display-hint-unix-timestamp

Conversation

@justahero
Copy link
Copy Markdown

@justahero justahero commented Oct 20, 2021

This PR implements #451.

The following log statement prints the given timestamp in milliseconds as a ISO8601 datetime without a timezone.

defmt::info!("Timestamp is {:iso8601ms}", 1618910624804);
// => "Timestamp is 2021-04-20T09:23:44.804Z"

Alternatively to log a u64 as a ISO8601 datetime in seconds use the :iso8601s display hint

defmt::info!("Timestamp is {:iso8601s}", 1618910624);
// => "Timestamp is 2021-04-20T09:23:44"

Copy link
Copy Markdown
Member

@japaric japaric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation looks good to me! Thinking more about it, the + specifier is a bit cryptic and it would be hardcoded to milliseconds. One may want to specify the timestamp in microseconds or some other resolution. So, I think we may want to call this one iso8601ms to differentiate from other possible future display hints. Thoughts?

@Urhengulas
Copy link
Copy Markdown
Member

Urhengulas commented Oct 20, 2021

[W]e may want to call this one iso8601ms to differentiate from other possible future display hints. Thoughts?

Is ms maybe enough? It seems more in line with the existing us (microseconds).

@justahero
Copy link
Copy Markdown
Author

The + display hint was more a suggestion, we can change it to something more appropriate. Imho the iso8601ms hint is a bit long but compared to ms it's less ambiguous. Reading the latter type hint, it kind of signals the full number is represented instead of a datetime.

Adapting the display hint to iso8601ms also allows a representation based on seconds, e.g. iso8601s which might also be useful.

@Urhengulas
Copy link
Copy Markdown
Member

Imho the iso8601ms hint is a bit long

My problem isn't actually the length, but I feel that it is rather cryptic. I definitely would need to look it up if I am seeing it in some elses code. And I would prefer "intuitiveness" over "shortness".

Other options I am thinking about are:

  • time
  • datetime
  • timestamp

Or on the short side:

  • t
  • dt
  • ts

But none of them seems really good.

@justahero
Copy link
Copy Markdown
Author

justahero commented Oct 21, 2021

@Urhengulas, from your list datetime seems the best choice as this is also defined in RFC 3339 to consists of a full-date (e.g. "2021-10-21") & full-time (e.g. "12:34:56.789").
In this regard ISO8601 is quite specific in its format, even though some components can be considered "optional" (e.g. fraction of a second or the time offset). My personal preference here is ISO8601, but I'm fine with other type hints as well.

@Urhengulas
Copy link
Copy Markdown
Member

@Urhengulas, from your list datetime seems the best choice as this is also defined in RFC 3339 to consists of a full-date (e.g. "2021-10-21" & full-time (e.g. "12:34:56.789").
In this regard ISO8601 is quite specific in its format, even though some components can be considered "optional" (e.g. fraction of a second or the time offset). My personal preference here is ISO8601, but I'm fine with other type hints as well.

Just go with your preferred one, then 😁

* split into two separate display hints, `:iso8601ms` for milliseconds &
  `:iso8601s` for seconds precision (similar to chrono)
@justahero justahero force-pushed the feature/display-hint-unix-timestamp branch from 089438a to 3cf29c6 Compare October 22, 2021 09:42
Copy link
Copy Markdown
Member

@Urhengulas Urhengulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good!

Comment thread decoder/src/frame.rs
Comment thread decoder/Cargo.toml Outdated
Comment thread decoder/src/frame.rs Outdated
Comment thread decoder/src/lib.rs
Comment thread firmware/qemu/src/bin/hints.rs
* incorporate review feedback
* update `chrono` dependency
* fix naming
@justahero justahero requested a review from Urhengulas October 25, 2021 12:15
@Urhengulas
Copy link
Copy Markdown
Member

bors d+

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Oct 26, 2021

✌️ justahero can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@justahero
Copy link
Copy Markdown
Author

bors r+

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Oct 27, 2021

Build succeeded:

@bors bors Bot merged commit 1bd226b into main Oct 27, 2021
@bors bors Bot deleted the feature/display-hint-unix-timestamp branch October 27, 2021 08:39
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