Skip to content

Enable UUID 1.0#329

Closed
marti4d wants to merge 3 commits intoSeaQL:masterfrom
marti4d:enable_uuid_1_0
Closed

Enable UUID 1.0#329
marti4d wants to merge 3 commits intoSeaQL:masterfrom
marti4d:enable_uuid_1_0

Conversation

@marti4d
Copy link
Contributor

@marti4d marti4d commented May 15, 2022

PR Info

Adds

  • Support for Uuid v1.0

Breaking Changes

  • Any downstream project will need to change uuid dependency to ^1, or change their code to use uuid_0() and such everywhere. Very likely we need a breaking-change version increase

@marti4d marti4d mentioned this pull request May 15, 2022
1 task
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you. I think it looks good.

@tyt2y3
Copy link
Member

tyt2y3 commented May 28, 2022

May be we shall release that in 0.26

use bigdecimal::BigDecimal;

#[cfg(feature = "with-uuid-0")]
use uuid_0::Uuid as Uuid0;
Copy link

Choose a reason for hiding this comment

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

So this means we can have both uuid@1 and uuid@0.8 enabled? 🤔

Copy link

Choose a reason for hiding this comment

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

I proposed this PR not to have this behavior and keep uuid as a single and unique dependency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Uuid

That 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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@ikrivosheev ikrivosheev Jun 1, 2022

Choose a reason for hiding this comment

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

@jdrouet I think you dont need to fix CI. I think is better fix code.

Copy link

@jdrouet jdrouet Jun 3, 2022

Choose a reason for hiding this comment

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

@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 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@tyt2y3
Copy link
Member

tyt2y3 commented Jul 1, 2022

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.
At the same time, I would suggest we also maintain the one previous release of sea-query. For example, let's say we upgrade uuid to 1 in 0.26 and drop uuid 0. In the meantime, we shall still backport features / fixes to sea-query 0.25 to ease the transition.

@tyt2y3 tyt2y3 mentioned this pull request Jul 1, 2022
@tyt2y3 tyt2y3 closed this Jul 1, 2022
hongkongkiwi added a commit to hongkongkiwi/sea-orm that referenced this pull request Jan 26, 2026
- Added DatabaseBackend::Cockroach variant
- Added cockroachdb feature flag
- Added schema generation for IDENTITY columns
- Added documentation and tests

Related: SeaQL/sea-query#329
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.

Enable UUID v1.0 for Postgres

4 participants