-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix most type-defs for #[auto_type]
by cleaning up all the type defs
#3783
Conversation
that should be almost always backwards compatible because the module was only pub(crate) anyway. It is not strictly backwards compatible because diesel-rs#3745 (comment)
diesel/src/query_dsl/mod.rs
Outdated
@@ -332,6 +332,7 @@ pub trait QueryDsl: Sized { | |||
where | |||
Self: methods::SelectDsl<CountStar>, | |||
{ | |||
#[allow(deprecated)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we choose that solution we need to think about if we really want to put a #[deprecated]
onto the old type aliases as this results in warnings as soon as you have an import like that, even if that import could refer to the function as well.
f26f7d2
to
9a04494
Compare
@Ten0 I think this is now mostly ready for review. It solves most of the issues, what's left could probably be documented as exception? |
9a04494
to
9a40ea6
Compare
This is on my to-do list, I need to find the time to have a look at it - not sure when, sorry. |
👍 Thanks for letting me know. Take your time, its not necessary to rush anything here as that feature is not released yet. |
9a40ea6
to
4402f6d
Compare
@Ten0 I've updated this PR to include tests for the |
4402f6d
to
47f2692
Compare
47f2692
to
32e1484
Compare
Sorry for the huge delay 😔 So this approach of updating the type aliases names to their PascalCase names has the downsides that:
For these reasons, I think I like more the idea of not changing our (weird) current convention with regards to naming type aliases for (If however we do decide to apply the approach drafted at #3773 we should still apply the other fixes that are in this PR, as explained in the PR description) |
There is also #3898 (and rust-lang/rust#114682) which might be relevant here, as the rustc devs might want to disallow some of that overlapping type import thing. That written: I think my main problem with the That written: Even if we go with that other approach we need to carefully inspect this PR for the other fixes that are included here. That includes for example using |
I recall having considered that but ended up proposing an implementation with
We could also decide to make the deprecated
Definitely yes :) |
…ype_max Conflicts: diesel_compile_tests/tests/fail/right_side_of_left_join_requires_nullable.stderr
so that users have the flexibility of exporting the helper type without exporting the function if they want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM apart from this minor naming thing ! 🎉
(Also I'm not sure the PR description is completely accurate anymore at this point)
diesel_derives/src/lib.rs
Outdated
/// | ||
/// SQL functions declared with this version of the macro will not be usable with `#[auto_type]` | ||
/// or `Selectable` `select_expression` type inference. | ||
#[deprecated = "Use [`define_sql_function`] instead"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want to add a since
note here - not sure when we want to release exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've set it to 2.2.0. I think we will release that version in a timely manner after I finished #3951 copy support)
Fixes #3745
This fixes most of the broken cases for
#[auto_type]
by:PascalCase
instead of lower case names (There are only 10 functions affected by that)Like
is now also used forBlob
or there is a singleConcat
operator instead of having 3 separate ones.)Insert
statements to allow them working with#[auto_type]
as well.limit
/offset
by just adding a dummy type parameter there.This is explicitly a WIP PR as it contains quite a few undocumented items that either need documentation or need to be moved into private modules. Everything included in this PR shouldn't be a breaking change.
This PR does not address the following issues with
#[auto_type]
:QueryDsl::into_boxed()
-> This requires a lifetime and a backend type. Likely hard to ever support?QueryDsl::count()
-> Name collision with thecount()
function typediesel::update()
-> There is an existing incompatibleUpdate
type indiesel::dsl
. I do not see an easy way to change that type into something that would work with#[auto_type]
diesel::select()
-> There is an existingSelect
type indiesel::dsl
that is used byQueryDsl::select()
diesel::sql_query()
-> does not have any (non-default) type parameter, but#[auto_type]
requires onediesel::dsl::sql()
-> requires the sql type to come from somewhere -> maybe#[auto_type]
can be updated to pass a explicit specified generic type forward as well (sodsl::sql::<Integer>("")
turns intodsl::Sql<Integer>
?)*ExpressionMethod
trait methods for postgres, as I've run out of time before tackling them