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

Add "with-time" feature #256

Closed
wants to merge 3 commits into from
Closed

Add "with-time" feature #256

wants to merge 3 commits into from

Conversation

cemoktra
Copy link

@cemoktra cemoktra commented Feb 8, 2022

with-time and with-chrono are mutually exclusive

Comment on lines 692 to 697
pub fn is_date_time_utc(&self) -> bool {
#[cfg(feature = "with-chrono")]
#[cfg(any(feature = "with-chrono", feature = "with-time"))]
return matches!(self, Self::DateTimeUtc(_));
#[cfg(not(feature = "with-chrono"))]
#[cfg(not(any(feature = "with-chrono", feature = "with-time")))]
return false;
}
Copy link
Member

@billy1624 billy1624 Feb 11, 2022

Choose a reason for hiding this comment

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

I think we could refactor all these feature guards by adding a marker feature, perhaps named _with-chrono-or-time. This feature will be turned on if either with-chrono or with-time was enabled.

Then, we could simply check

#[cfg(feature = "_with-chrono-or-time")]
#[cfg(not(feature = "_with-chrono-or-time"))]

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 it's fine either way

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Btw... we should also tests if time can be binded on various db drivers or not.
https://github.com/SeaQL/sea-query/tree/master/src/driver

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 10, 2022

Btw... we should also tests if time can be binded on various db drivers or not. https://github.com/SeaQL/sea-query/tree/master/src/driver

what is the support matrix?

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 10, 2022

Thank you so much for the contribution. I have a clearer thought by now:

SO the problem is that it creates confusion. Cargo features are not supposed to be mutually exclusive, although quite a number of crates does so. Also, all-features does not compile.
But inevitably if some crate turned on the time feature there might be another crate not expecting that and result in compilation failure.

It will be hard to maintain in the long run if we support the two using mutually exclusive feature flags.

What I have in mind is may be we can have a separate "facade" crate that abstracts the two libraries and provide a uniform interface for SeaQuery (and other downstream crates) to consume

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 10, 2022

Actually, it might makes more sense to support both to coexist at the same time.

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 11, 2022

Actually, it might makes more sense to support both to coexist at the same time.

This should be easier and a once off breaking change right? @billy1624

@billy1624
Copy link
Member

Actually, it might makes more sense to support both to coexist at the same time.

This should be easier and a once off breaking change right? @billy1624

Yeah, this would be easier to just add time types as new Value's variants, alongside with chrono's variants. It will be a breaking change but I think it's fine as long as we keep all old Value variants untouched. This is easy to fix for anyone matching the Value Enum.

@tyt2y3
Copy link
Member

tyt2y3 commented Mar 11, 2022

Sounds good

@billy1624 billy1624 marked this pull request as draft March 12, 2022 02:15
@tyt2y3
Copy link
Member

tyt2y3 commented Mar 13, 2022

Closed in favour of #267

@tyt2y3 tyt2y3 closed this Mar 13, 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.

None yet

3 participants