Merge/diesel dynamic schema #2478
Conversation
The test for this feature is a bit wonky. This feature only matters for PG, but I don't want to dick around with setting up test suites for multiple backends on such a simple crate. Instead we can just test the generated SQL directly.
Allow specifying schema names other than the default
Add a licence for the crate
Create .travis.yml
* Add documentation with examples * Hide example code Co-Authored-By: Georg Semmler <[email protected]> * Hide test code Co-Authored-By: Georg Semmler <[email protected]> * Destructure results Co-Authored-By: Georg Semmler <[email protected]> * Destructure results Co-Authored-By: Georg Semmler <[email protected]> * Reword help because of upcoming new functionality * Reword comment to make it simpler * Reword help to omit imprecise arguments * Fix examples with destructuring * Reword help to focus on core idea * Rework help to add links * Rework help example to add asserts
weiznich
force-pushed
the
merge/diesel_dynamic_schema
branch
from
eb404b5
to
79f4190
Compare
There was a problem hiding this comment.
Hi, it looks good to me. I did the tests and all seem to pass, which is great. I added a suggestion to remove a clippy warning too.
I also noticed that in the Cargo.toml file, the diesel dependency is shown as 1.0.0, but in the rest of the crates in the workspace, it's shown like this:
[dependencies.diesel]
version = "~2.0.0"
path = "../diesel"
default-features = falseShould this be changed?
Finally, as far as I can tell this does not include the changes in diesel-rs/diesel-dynamic-schema#10, right? That would personally be a really nice feature to have. I've been trying to make it work with the latest 2.0.0 diesel branch, but I'm too far from understanding the inner workings of diesel to make it compile.
In any case, this looks pretty good from my side. Let me know if there is something else I should be looking at :)
|
@Razican Thanks for the review. I will try to address the open points after my holidays (so not for before September).
Good point. Can you add some comment in the corresponding
That's intentionally as I first want to merge both repositories. After that we can start adding new features. I will likely do a followup PR with those changes later. |
Co-authored-by: Iban Eguia <[email protected]>
weiznich
force-pushed
the
merge/diesel_dynamic_schema
branch
from
486b56a
to
29e5925
Compare
weiznich
force-pushed
the
merge/diesel_dynamic_schema
branch
from
29e5925
to
eed7f2a
Compare
I propose merging
diesel_dynamic_schemainto the diesel main repository as this makes it much easier to coordinate the necessary changes between those two crates to implement dynamic select clause support.