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

Compatibility with diesel master after 72bfb356 #64

Merged
merged 9 commits into from
Dec 19, 2021

Conversation

Ten0
Copy link
Contributor

@Ten0 Ten0 commented Jul 8, 2021

diesel-rs/diesel#2787 introduced ability for diesel CLI to infer unknown SQL types from the Postgres backend, and generate corresponding SQL types.

This PR aims to support plugging into these auto-generated types from the schema, completely removing the need for patching the diesel schema if using the Postgres backend.

The approach is:

  • For Postgres, use the diesel-cli-generated SqlType structures.
  • For other backends, keep generating the same structure as before.

@adwhit
Copy link
Owner

adwhit commented Jul 9, 2021

Thanks, this is really cool.

The README/docs will have to change quite a bit to explain these changes. This is unfortunate as the current instructions on master still apply to v1.X, which means that new users don't have dig to find what they need, and I would like to keep it that way.

So, I am minded to merge these changes, but then rename the branches like:

master -> diesel-next
diesel-1 -> master

I probably should have done that in the first place, but I didn't realise that diesel-2 would be a mutli-year project.

What you think?

@Ten0
Copy link
Contributor Author

Ten0 commented Jul 10, 2021

So, I am minded to merge these changes, but then rename the branches like:

master -> diesel-next
diesel-1 -> master

That makes some sense and wouldn't be at all an issue for me.
However we may want to think of:

  • That wouldn't be very homogeneous with diesel, which eventually will have a diesel-1.x branch, a diesel-2.x branch, and a master branch
  • I'm not sure how many people have their Cargo.toml set to diesel-derive-enum = { git = "https://github.com/adwhit/diesel-derive-enum" }, which would all need to update (although that probably wouldn't be very hard for them).
  • I feel like the best place for this documentation that is version-specific is actually here:

Overall I think it's pretty hard to guarantee that the master branch's Readme corresponds to the latest released API so it shouldn't be assumed by the users nor be considered a requirement.
I don't have a very strong opinion on this though.

@Ten0
Copy link
Contributor Author

Ten0 commented Dec 16, 2021

I'd like to open a separate PR for latest diesel changes.
By the way diesel 2.0 is approaching.

@adwhit
Copy link
Owner

adwhit commented Dec 17, 2021

Hi sorry my bad, I let this PR dangle way too long. Would you prefer to merge this first before doing a follow-up or just create a new PR with all the changes in it? I should have time to address it this weekend.

@Ten0
Copy link
Contributor Author

Ten0 commented Dec 19, 2021

I'd prefer to merge this one first 🙂
I've got the other one ready to be rebased on the merge and opened. 🙂

@adwhit adwhit merged commit a339d70 into adwhit:master Dec 19, 2021
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.

2 participants