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

Adds features option to use either time or chrono for sqlx #46

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

karuna
Copy link
Contributor

@karuna karuna commented Sep 2, 2024

Adds features option to use either time or chrono for sqlx

- Adds cfg option to use chrono instead of time with sqlx
@karuna karuna mentioned this pull request Sep 2, 2024
@maxcountryman
Copy link
Owner

Thanks for this. I can't publish it until later, but I'll go ahead and merge.

@maxcountryman maxcountryman merged commit d3be5e3 into maxcountryman:main Sep 2, 2024
1 of 3 checks passed
@maxcountryman
Copy link
Owner

Oops, I missed that this breaks feature unification. I'll have to roll this back. I'm not sure what the "right" approach is, but note that sqlx doesn't break feature unification so I'd prefer that we don't either. I'm going to revert this for the time being.

maxcountryman added a commit that referenced this pull request Sep 3, 2024
This would break feature unification.

This reverts commit d3be5e3.
@abonander
Copy link

@maxcountryman I would recommend maybe just using the chrono feature, as it's lower in precedence than the time feature and will never override it.

Had we better understood feature unification when we were initially designing SQLx, we might have avoided this issue entirely, but the choice is rather ossified at this point.

Even once launchbadge/sqlx#3383 is fully implemented, I'm not sure what to do with the existing behavior except maybe force the user to pick an option.

@aschweig
Copy link

aschweig commented Sep 7, 2024

I had the idea to trivially take the outer-product of the various options, so one has both feature = "sqlite" and feature = "sqlite-chrono", and the latter introduces the SqliteChonoStore; and similarly for mysql, and prostgres . The feature (time or chrono) is simply passed on to sqlx without thinking too much -- though I am thinking maybe I just switch to time. (I lifted most of my work from PR #46)

https://github.com/aschweig/tower-sessions-stores/tree/chronofix/sqlx-store

@nicolasauler
Copy link

Hey guys, has this been fixed? Or, can we fix it?

@karuna
Copy link
Contributor Author

karuna commented Oct 24, 2024

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.

6 participants