Enable UUID 1.0#329
Conversation
tyt2y3
left a comment
There was a problem hiding this comment.
Thank you. I think it looks good.
|
May be we shall release that in 0.26 |
| use bigdecimal::BigDecimal; | ||
|
|
||
| #[cfg(feature = "with-uuid-0")] | ||
| use uuid_0::Uuid as Uuid0; |
There was a problem hiding this comment.
So this means we can have both uuid@1 and uuid@0.8 enabled? 🤔
There was a problem hiding this comment.
I proposed this PR not to have this behavior and keep uuid as a single and unique dependency.
There was a problem hiding this comment.
Ah -- Your PR is very similar to my original change when I first proposed my PR. The issue I ran into -- and you will see this when you remove the "draft" keyword and pre-commit testing is done -- is that you've now created mutually-exclusive features (cargo docs about this)
But we run all our tests with cargo test --all-features, and so you will see errors when that is run.
I had already suggested instead that we should have a "pecking order", where if UUID-1 is enabled it will automatically upstage UUID-0. I remember the issue being that sprinkling these conditional blocks everywhere gets kind-of nasty:
#[cfg(all(not(feature = "with-uuid-1"), feature = "with-uuid-0"))]
use uuid_0::Uuid as Uuid;
#[cfg(feature = "with-uuid-1")]
use uuid_1::Uuid as Uuid;
#[cfg(any(feature = "with-uuid-1", feature = "with-uuid-0"))]
// Do thing with UuidThat being said, maybe now that @tyt2y3 has seen my PR and the juggling that has to be done to enable both at the same time, they may prefer that. But I think at-least you'll have to add a few more conditionals to implement this upstaging-behavior.
There was a problem hiding this comment.
I ran into this problem when trying to upgrade time in this PR and I'm trying to find a way to fix the CI.
There was a problem hiding this comment.
@jdrouet I think you dont need to fix CI. I think is better fix code.
There was a problem hiding this comment.
@ikrivosheev do you think we should be able to have uuid@0.8 and uuid@1 at the same time?
For the time related PR, I'm still working on it 😉
There was a problem hiding this comment.
On one hand: CI on the other hand - very strange behavior when we compile one library with a different version...
I think it is better to panic when both features are enabled.
|
Um... actually I have a worse idea. Maintaining multiple major versions in the same library seems not to be so scalable. After a some thought, I think we should ONLY support the latest major release of a library. |
- Added DatabaseBackend::Cockroach variant - Added cockroachdb feature flag - Added schema generation for IDENTITY columns - Added documentation and tests Related: SeaQL/sea-query#329
PR Info
Adds
Breaking Changes
uuiddependency to^1, or change their code to useuuid_0()and such everywhere. Very likely we need a breaking-change version increase