Chrono types for Timestamp

[durangatan]

Description

Fixes #35.

• Each branch of the Into implementation casts everything to a DateTime<Utc> with at least millisecond precision. I figure the caller can cast the value to a timezone if needed.
• I'm not sure if I'm representing Timestamps the preferred way in UTC-land, because I'm not super familiar with how influxdb works. For example, Timestamp::Hours(3) probably doesn't mean three hours from the epoch, but for now I kept it consistent.
• The From<DateTime> implementation creates a Timestamp::Milliseconds from a given date. It will work for any DateTime object that implements TimeZone.
• these implementations are behind the chrono_timestamps compilation feature.

I hope this is sort of what you're looking for; let me know if you want me to take it in a different direction or implement more chrono converters.

Checklist

• Formatted code using cargo fmt --all
• Linted code using clippy cargo clippy --all-targets --all-features -- -D warnings
• Updated README.md using cargo readme > README.md
• Reviewed the diff. Did you leave any print statements or unnecessary comments?
• Any unfinished work that warrants a separate issue captured in an issue with a TODO code comment

[msrd0]

Why do you always enable the chrono feature in test by all those any(test, feature = "chrono_timestamp")? We test with cargo test --all-features so I guess it's fine to only enable the test when the feature is enabled

[Empty2k12]

Suggested change

chrono = { version = "0.4.9" }

[Empty2k12]

@durangatan this one and we're ready to merge ;)

[msrd0]

This should be feature-gated (see above)

[Empty2k12]

Thanks for this PR! 🎉

For example, Timestamp::Hours(3) probably doesn't mean three hours from the epoch, but for now I kept it consistent.

Sadly, this is exactly the way it is. Time in InfluxDb is UTC and an Unix nanosecond timestamp by default. We can set other precisions (which is done using the Timestamp enum).

The From implementation creates a Timestamp::Milliseconds from a given date. It will work for any DateTime object that implements TimeZone

It should probably use Timestamp::Nanoseconds by default, as that's the preferred time resolution from InfluxDb

It would probably also be nice to document the feature gate somewhere, so potential users can find this easily.

[msrd0]

Btw, right now you only implement DateTime<Utc> and I guess this makes sense not to have arbitrary Timezones but have the user convert them first. But would it make sense to support NaiveDateTime if the user explicitly don't care about timezones?

[durangatan]

I chose UTC because it is a "true unix timestamp".
from the docs for NaiveDateTime.timestamp_nanos:

Note that this does not account for the timezone! The true "UNIX timestamp" would count seconds since the midnight UTC on the epoch.

lmk if I should change the implementation to NaiveDateTime

[msrd0]

I see your point, but for some user who doesn't care about the timezone and interprets the Utc time as if it was their local one, it might make sense to also support NaiveDateTime. @Empty2k12 what do you think?

[durangatan]

Based on the above comments in the thread, I think it's a safe assumption that a user will expect an influxDB timestamp to represent a UTC datetime, not a timezoned time.
"Time in InfluxDb is UTC and an Unix nanosecond timestamp by default. We can set other precisions (which is done using the Timestamp enum)."

[Empty2k12]

I would say that supporting UTC only is reasonable, as InfluxDb makes that assumption for us and we should strive to behave similar to it.

[durangatan]

not sure how to fix the last bit of the build; looks like kcov is failing but I can't tell which command specifically fails

[Empty2k12]

From the logs, it looks like cargo-travis is not being run with --all-features. I think this argument is missing in these two lines: https://github.com/Empty2k12/influxdb-rust/blob/98820c7a1d369cce6ea23a7489fb544e92b179b9/.travis.yml#L54-L55

Since I'm planning on removing coverage in #23 since it's broken and gives inaccurate results I'd be fine with you also removing the cargo-travis step and all references to code coverage (lib.rs badge and Cargo.toml badge)

[Empty2k12]

Looks good to me! Thanks for contributing this!

Since @msrd0 requested the feature, I'll have him have the last words.

Some small suggestions have been noticed.

Stale

[Empty2k12]

Thank you very much for this contribution. 🎉
This will be helpful to everyone working with Chrono in their projects.