-
Notifications
You must be signed in to change notification settings - Fork 1.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
Upgrade to time 0.3 #1455
Upgrade to time 0.3 #1455
Conversation
It doesn't look like macros like it when time is renamed, since they all import it with Any ideas? |
Can we get around using the macros? |
We could but first I'm trying to see if there are other solutions time-rs/time#356 (reply in thread) |
@paolobarbolini sounds like a solution wasn't forthcoming there. I dunno how feasible it is to try to get Can we just inline the output of the macros here? It seems like it should be relatively stable as the code that |
I am not familiar with this issue and am not sure the circumstances are the same, but is the method used by the |
I suggested it but they don't seem too convinced about it time-rs/time#356 (reply in thread) I'll try inlining the macro output.
That's what we're doing too |
@paolobarbolini sorry, I didn't link to the correct spot. I meant with how they declare time as an extern crate. Doing something similar, I was able to run The specific changes I made can be viewed here: kyle-mccarthy@005d922 |
Interesting, I didn't know extern crate had the power to rename a crate like that. |
I cherry-picked your commit @kyle-mccarthy. I got a warning that the |
sqlx-core/src/lib.rs
Outdated
@@ -104,3 +104,6 @@ pub mod mssql; | |||
/// sqlx uses ahash for increased performance, at the cost of reduced DoS resistance. | |||
use ahash::AHashMap as HashMap; | |||
//type HashMap<K, V> = std::collections::HashMap<K, V, ahash::RandomState>; | |||
|
|||
#[cfg(feature = "time-03")] | |||
extern crate time_03 as time; |
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.
Can we make this conditional on feature = "postgres"
as well? It should fix those "unused" warnings.
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.
Or nevermind this if you're still working on support for MSSQL/SQLite
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.
Honestly I just tested postgres. I haven't looked into mysql and sqlite, I just fixed the compiler warnings 😄. From a quick search it looks like support for time
in sqlite hasn't been implemented yet!? https://github.com/launchbadge/sqlx/tree/master/sqlx-core/src/sqlite/types
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.
It definitely looks to be that way, I guess no one's ever asked for it. I guess don't worry about SQLite then?
So yeah, whatever suppresses the warning works for me.
I am absolutely ecstatic that this just works! This is a backwards-compatible change too, isn't it? So we could release this with 0.5.10 instead of 0.6.0 |
@paolobarbolini do you think you'll be able to finish this soon? |
Yes, I'll look into it next week. |
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.
Couple remaining nits:
- can we fix the warnings on the
extern crate time_03 as time;
insqlx-core/src/lib.rs
when it's not being used? - it looks like the existing type tests for
time 0.2
were repurposed to test the types fromtime 0.3
which leaves the former with no type tests, can we make sure to cover both?
Ah right. I'll look into it |
I'm trying to do point 2 but we're again running into the same problem we had before with both time 0.2 and time 0.3 macros importing time as |
It seems to me like these macro issues are going to come up again when time 0.4 comes out. Just to get an idea I looked at the downloads for the latest 0.2 1 and 0.3 2 versions and between the two time 0.3 only makes ~30% of the downloads 😞. I kinda wished everybody updated so we didn't have to do this. Footnotes |
Also I just realized the sqlx-macros optional feature suggestion system should be updated to support both versions 😅 |
@paolobarbolini should we just bite the bullet and upgrade to |
b4eef14
to
8b1d812
Compare
👍 I removed all of my changes and rebased the original author's changes on top of master |
It might be worth to update the |
Officially, we only support the latest stable version of Rust. I try to avoid using language or library features that are too new but MSRV just isn't something we keep track of on this project. Should probably add a disclaimer somewhere visible for that. |
This is a lib (hence no |
very interested in this! |
The time crate docs say Libraries should never enable this feature, as the decision of what format to use should be up to the user |
Even if we wanted to enable the feature by default, which we don't for the reason @paolobarbolini stated, I have to question
Which is ever so close to being ISO-8601/RFC 3339 compliant but I believe the space separator for the offset breaks it. |
Indeed, it produces dates like
which are rejected by Javascript's You can enable RFC-3339 serialization, but you have to mark every The reasoning for the choice is explained here: time-rs/time#248 (comment) I don't see why RFC 3339 couldn't have been the default since it's generally more useful for most of the places you'd actually use |
Thanks for the comments and the correct way to serialize with serde (it all makes sense now). |
@@ -151,7 +151,7 @@ sha-1 = { version = "0.9.8", default-features = false, optional = true } | |||
sha2 = { version = "0.9.8", default-features = false, optional = true } | |||
sqlformat = "0.1.8" | |||
thiserror = "1.0.30" | |||
time = { version = "0.2.27", optional = true } | |||
time = { version = "0.3.2", features = ["macros", "formatting", "parsing"], optional = true } |
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.
Wouldn't #[serde(with = "time::serde::rfc3339")]
(as described in this comment) require the serde-well-known
feature flag? Since serializing with RFC3339 format is a very common use case, I guess adding serde-well-known
would make sense...
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.
Generally you don't want to enable feature flags you don't strictly need. That would force that feature on for all crates that depend on SQLx.
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.
Makes sense... but what is then the recommended way to read a timestamp from the database and put it into a DTO in RFC3339 format? Since the version of time
that sqlx comes with doesn't support this (due to version and that feature flag), a user of sqlx has to use an intermediate "DatabaseDTO" with sqlx::types::time::OffsetDateTime
for fetching from the database (using query_as
), just to then convert it manually into an "ActualDTO" with time::OffsetDateTime
(I did that with time::OffsetDateTime::from_unix_timestamp(db_dto.created_at.unix_timestamp())
, but I'm not sure that's the best way). Only then the aforementioned way to serialize a timestamp using RFC3339 with with
actually works. Or am I missing something?
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.
That's only an issue because SQLx is currently using a different version of time
than you are. Otherwise sqlx::types::time::OffsetDateTime
and time::OffsetDateTime
should always refer to the same type.
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.
time::serde::rfc3339
exists since time
v0.3.6, so it's not available in the version proposed in this PR (which means serde-well-known
would only make sense when using >= 0.3.6 anyway; before that, there is no such feature flag).
I get that enabling too many features is not a good idea as it would impact lots of users. But what about going with a more recent version of time, i.e., >=0.3.6? That would make it easier for anyone trying to include timestamps in REST API responses :)
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.
time::serde::rfc3339
exists sincetime
v0.3.6, so it's not available in the version proposed in this PR
sqlx depending on 0.3.2
means versions >=0.3.2, <0.4.0
are allowed, so you're free to use features from newer patch versions of the time crate
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.
The version specified here is not restrictive and Cargo will automatically pull in the latest 0.3.x
release if it's not in your Cargo.lock
already when building your crate.
You should also specify time = { version = "0.3.6", features = ["serde-well-known"] }
in your own dependencies to be certain of this.
Continuation of #1444
Still left to do:
Closes #1277