Skip to content
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 time dependency version to 0.3 #341

Closed
wants to merge 7 commits into from

Conversation

jdrouet
Copy link

@jdrouet jdrouet commented May 30, 2022

The time crate version 0.3.x as been released for a while now and sea-orm is still on version 0.2, blocking the integration with tools like async-graphql which relies on 0.3.x.

PR Info

Adds

  • create a feature with-time-0_2 to use time@^0.2
  • create a feature with-time-0_3 to use time@^0.3

Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@jdrouet hello! Thank you for contributation. I think we need support both time = 0.2 and time = 0.3 Because postgres-types has support both time version:

postgres-time = ["with-time", "postgres-types/with-time-0_2"]

An example you can see here: #329

@jdrouet
Copy link
Author

jdrouet commented May 31, 2022

@krivosheev I'm trying to do as you suggest but run into a problem.
Having both versions in the Cargo.toml, we'd mean aliasing time@0.2 to time-0_2 and time@0.3 to time-0_3.
Which means that for the imports we'd have to do use time_0_2 and use time_0_3.
The problem is that when we use macros like date, time and offset, the procedural macro makes a reference to ::time::... which is not available in our case because it's aliased to time_0_x.
In my case, only the use of format_description is causing a problem for the library and date, time, and offset in the tests.
postgres-types doesn't seem to have this issue because they are not using any procedural macro.
My next iteration will be to remove those macros.

Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
@jdrouet jdrouet requested a review from ikrivosheev May 31, 2022 09:04
Copy link
Member

@ikrivosheev ikrivosheev left a comment

Choose a reason for hiding this comment

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

@jdrouet awesome! Small comment.

src/lib.rs Outdated
@@ -741,6 +741,7 @@
)]

pub mod backend;
pub mod deps;
Copy link
Member

Choose a reason for hiding this comment

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

I think deps should be pub(crate), because this is internal mod.

Copy link
Author

Choose a reason for hiding this comment

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

well, we need to expose it to use it in tests/*

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, maybe we need to separating the tests for different time versions

Copy link
Member

Choose a reason for hiding this comment

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

Or re-import this mod in tests_cfg

Copy link
Member

Choose a reason for hiding this comment

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

My point was: when we were making some internal mod public it became part of the public API sea-query and every change is breaking...

Copy link
Author

Choose a reason for hiding this comment

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

sure, I'll try to fix that. For now I'm trying to find a nice way to fix the CI 😉

@ikrivosheev
Copy link
Member

ikrivosheev commented May 31, 2022

@jdrouet some problems with tests, because tests run with: --all-features

Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
…ther

Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
Signed-off-by: Jérémie Drouet <jeremie.drouet@gmail.com>
@jdrouet jdrouet mentioned this pull request Jun 1, 2022
2 tasks
@ikrivosheev
Copy link
Member

@jdrouet hello! Thank you for PR. We have done in: #356

@ikrivosheev ikrivosheev closed this Jul 1, 2022
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.

upgrade time to 0.3.x version
2 participants