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

extend pg lower function to support Multirange #4179

Merged
merged 2 commits into from
Aug 23, 2024

Conversation

guissalustiano
Copy link
Contributor

@guissalustiano guissalustiano commented Aug 17, 2024

Extending the work of #4159, this PR updates one Range function which also support multi-range.
This is a small PR to check the format, once approved I'm going to implement for the other methods.
I also add more example testing nullability and simplify the example removing the table and only use the select.

@guissalustiano guissalustiano force-pushed the multirange_lower_function branch from 0911151 to 64c9302 Compare August 17, 2024 14:22
@guissalustiano
Copy link
Contributor Author

The autotype test is broken

error[E0107]: type alias takes 2 generic arguments but 1 generic argument was supplied
   --> diesel_derives/tests/auto_type.rs:395:9
    |
395 |         lower(pg_extras::range),
    |         ^^^^^ ---------------- supplied 1 generic argument
    |         |
    |         expected 2 generic arguments
    |
note: type alias defined here, with 2 generic parameters: `R`, `T`
   --> /home/guiss/oss/diesel/diesel/src/pg/expression/helper_types.rs:314:10
    |
314 | pub type lower<R, T> = super::functions::lower<SqlTypeOf<R>, SqlTypeOf<T>, R>;
    |          ^^^^^ -  -
help: add missing generic argument
    |
395 |         lower(pg_extras::range, T),

I tried to follow the array_append example but it didn't work. Could someone help-me with that?

@weiznich weiznich requested a review from a team August 18, 2024 14:47
@guissalustiano guissalustiano force-pushed the multirange_lower_function branch from 64c9302 to d0f0494 Compare August 19, 2024 15:21
/// # Ok(())
/// # }
/// ```
#[cfg(feature = "postgres_backend")]
fn lower<T: RangeHelper>(range: T) -> Nullable<<T as RangeHelper>::Inner>;
fn lower<Rang: MultirangeOrRangeMaybeNullable<Inner=T> + SingleValue, T: SingleValue>(range: Rang) -> Nullable<T>;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn lower<Rang: MultirangeOrRangeMaybeNullable<Inner=T> + SingleValue, T: SingleValue>(range: Rang) -> Nullable<T>;
fn lower<Rang: MultirangeOrRangeMaybeNullable + SingleValue>(range: Rang) -> Nullable<Rang::Inner>;

We should be able to remove the T type here in favor of using the associated type as return type directly. That should hopefully also allow the type lower to be unchanged and remain compatible with #[auto_type]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 🙏🏽 !

@guissalustiano guissalustiano force-pushed the multirange_lower_function branch from d0f0494 to 4e8b1f1 Compare August 21, 2024 08:52
@weiznich weiznich added this pull request to the merge queue Aug 23, 2024
Merged via the queue into diesel-rs:master with commit 01c2eae Aug 23, 2024
48 checks passed
@guissalustiano guissalustiano deleted the multirange_lower_function branch August 23, 2024 13:42
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