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

Faillible Queryable #2599

Merged
merged 10 commits into from
Jan 27, 2021
Merged

Faillible Queryable #2599

merged 10 commits into from
Jan 27, 2021

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Dec 21, 2020 •
edited
Loading

Fixes #2523

TODO:

Ten0 added a commit to Ten0/diesel-derive-enum that referenced this pull request Dec 21, 2020
@Ten0
Faillible queryable support
9306544 
diesel-rs/diesel#2599
@weiznich
Copy link
Member

As with #2559 I would like to see a complete port of the repo before deciding anything else here. Generally speaking I think that one may be a better solution than the other PR, but I'm not entirely sure yet.

@Ten0
Copy link
Member Author

Ten0 commented Dec 28, 2020

I beleive that's it :)

@weiznich weiznich added the run-benchmarks Used to indicate that github actions should run our benchmark suite label Jan 17, 2021
@weiznich
Copy link
Member

@Ten0 Could you please cherry-pick cba105b to fix the benchmarks? (Also for the other variant)

@weiznich weiznich added run-benchmarks Used to indicate that github actions should run our benchmark suite and removed run-benchmarks Used to indicate that github actions should run our benchmark suite labels Jan 22, 2021
weiznich
weiznich reviewed Jan 22, 2021
View reviewed changes
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

After reviewing both PR's I find that one the better solution. With the suggested change we could even make that not-breaking for downstream code with a minimal maintenance burden on our side.
I still would like to get a benchmark run on this (seems like the CI was not fixed with the last commit, I will push a new fix to master.).

diesel/src/deserialize.rs Show resolved Hide resolved
@weiznich weiznich mentioned this pull request Jan 22, 2021
Closed
@Ten0
Copy link
Member Author

Ten0 commented Jan 22, 2021

After reviewing both PR's I find that one the better solution.

That's also what I thought after experimenting with the other one. 🙂

@Ten0 Ten0 marked this pull request as ready for review January 22, 2021 17:21
@Ten0 Ten0 changed the title Faillible Queryable - Still generic Queryable version experiment Faillible Queryable Jan 22, 2021
@weiznich weiznich merged commit 9635290 into diesel-rs:master Jan 27, 2021
@Ten0 Ten0 deleted the faillible_queryable_stillgeneric branch January 27, 2021 12:54
@Ten0 Ten0 mentioned this pull request Jan 27, 2021
Merged
@Ten0
Copy link
Member Author

Ten0 commented Jan 28, 2021

Fork dropped, we're finally back to following diesel master, woohoo! 😊
Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiznich weiznich weiznich approved these changes

Assignees
No one assigned
Labels
run-benchmarks Used to indicate that github actions should run our benchmark suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No way to express inconsistency when deserializing for Queryable data since #2182
2 participants
@Ten0 @weiznich