Skip to content

relative time as timestamp #15

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

Closed
arvidn opened this issue Oct 24, 2017 · 11 comments
Closed

relative time as timestamp #15

arvidn opened this issue Oct 24, 2017 · 11 comments

Comments

@arvidn
Copy link
Contributor

arvidn commented Oct 24, 2017

The message timestamp is expressed as a relative time duration (std::chrono::milliseconds) here.

That seems like the wrong type, it really should be a time_point, right?

@mfontanini
Copy link
Owner

The timestamp is expressed as a relative time since epoch. So it's arguably the wrong type but not necessarily.

@rriggs
Copy link

rriggs commented Oct 24, 2018

So, we expect everyone to write:

.timestamp(duration_cast<milliseconds>(system_clock::now() - system_clock::from_time_t(0)))

Whenever they need a timestamp? That seems less than ideal.

@accelerated
Copy link
Contributor

.timestamp(duration_cast<milliseconds>(std::chrono::system_clock::now().time_since_epoch())) should do.

@accelerated
Copy link
Contributor

.timestamp(duration_cast<milliseconds>(std::chrono::system_clock::now().time_since_epoch())) should do.

But I agree that .timestamp(std::chrono::system_clock::now()) would be more user-friendly. I wonder if @mfontanini is ok with adding an overload to this function taking a time_point?

@mfontanini
Copy link
Owner

Yeah that should work.

@mfontanini
Copy link
Owner

Also, note that you don't have to specify anything if you want now to be the timestamp in the message. Messages will have now as the timestamp by default so you should only specify one if you want to override it (e.g. you want the timestamp to be something that comes out of the message's contents or something like that).

@accelerated
Copy link
Contributor

Yeah that should work.

@mfontanini, You mean adding the overload or the code snipped I posted?

@mfontanini
Copy link
Owner

Yep, I was referring to that comment.

@mfontanini
Copy link
Owner

Sorry, I meant your code snippet :)

@mfontanini
Copy link
Owner

I mean, adding the overload. Sorry, for the spam!

@accelerated
Copy link
Contributor

See PR#128.

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

No branches or pull requests

4 participants