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.

Sign up for GitHub

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

Jump to bottom

Fix type info access in Any database driver #1848

Merged
merged 5 commits into from Jul 14, 2022
Merged

Fix type info access in Any database driver #1848

merged 5 commits into from Jul 14, 2022

Conversation

raviqqe
Copy link
Contributor

@raviqqe raviqqe commented May 1, 2022 •
edited

Close #1408.

The Type<Any>::type_info() is not implemented currently and doesn't seem to be any time soon.

sqlx/sqlx-core/src/any/type.rs

Line 11 in 826e63f

// FIXME: nicer panic explaining why this isn't possible

abonander
abonander requested changes Jun 8, 2022
View reviewed changes
sqlx-core/src/error.rs Outdated
@@ -138,9 +138,8 @@ impl Error {
pub(crate) fn mismatched_types<DB: Database, T: Type<DB>>(ty: &DB::TypeInfo) -> BoxDynError {
// TODO: `#name` only produces `TINYINT` but perhaps we want to show `TINYINT(1)`
format!(
"mismatched types; Rust type `{}` (as SQL type `{}`) is not compatible with SQL type `{}`",
Copy link
Collaborator

@abonander abonander Jun 8, 2022 •
edited

Choose a reason for hiding this comment

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

I'm loathe to lose this information as I think it could be really helpful with debugging.

I think instead we should fix this one level up, calling this function with the underlying DB type instead of Any:

I'd probably make a version of this function as a method on AnyTypeInfo and match on AnyTypeInfoKind, then you can just call that method from the overridden trait methods above.

raviqqe
raviqqe commented Jun 11, 2022
View reviewed changes
sqlx-core/src/any/row.rs Outdated Show resolved Hide resolved
@raviqqe raviqqe requested a review from abonander June 11, 2022 22:23
@abonander
Copy link
Collaborator

If you rebase it should fix the spurious CI failures.

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 13, 2022

@abonander Can you run the workflow? Thanks!

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 13, 2022

Do you think the test failure is due to my changes in this PR?

@abonander
Copy link
Collaborator

You need to rebase on main and it should fix them. If you did that then main in your local clone is probably out of date.

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 13, 2022 •
edited

Ah sorry, I was rebasing against the master branch instead of main!

It should be up to date now.

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 14, 2022 •
edited

Also, feel free to edit my branch if necessary.

@abonander
Copy link
Collaborator

Sorry, one more rebase should do it.

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 14, 2022

Done!

@abonander abonander merged commit 9534de3 into launchbadge:main Jul 14, 2022
@abonander
Copy link
Collaborator

Thanks!

@raviqqe
Copy link
Contributor Author

raviqqe commented Jul 14, 2022

Thank you so much for merging this too!!! 😄

@raviqqe raviqqe mentioned this pull request Aug 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abonander abonander Awaiting requested review from abonander

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

unimplemented! in typeinfo blocks to get any detail about error
2 participants
@raviqqe @abonander