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

feat: add method find_both_related() #1633

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ProbablyClem
Copy link
Contributor

@ProbablyClem ProbablyClem commented May 8, 2023

PR Info

New Features

  • new selector SelectBoth in which the return type is Vec<(E::Model, F::Model)>
  • a new method fn select_both<F>(mut self, _: F) -> SelectBoth<E, F> on Select
  • new method fn find_both_related()<R>(self, r: R) -> SelectBoth<E, R>

@ProbablyClem ProbablyClem changed the title feat: add method inner_join_and_select feat: add method inner_join_and_select May 8, 2023
@Pascualex
Copy link

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

@ProbablyClem
Copy link
Contributor Author

I'm not familiar with the internals of SeaORM, but this looks like a very direct and clean conversion of the implementation for select_also.

Wouldn't it make sense to also add a find_both_related method? Thank you for your contribution.

Thanks @Pascualex
Yeah, I do like that name, as it matches the find_xxx_related as talked in the issue.
Do you think I should rename the inner_join_and_select or create a new one @tyt2y3 ?

@ProbablyClem
Copy link
Contributor Author

I renamed it for now, as I think find_both_related is a better name.
I can still go back to inner_join_and_select if wanted just let me know

@ProbablyClem ProbablyClem changed the title feat: add method inner_join_and_select feat: add method find_both_related() May 18, 2023
src/query/join.rs Outdated Show resolved Hide resolved
Copy link
Member

@tyt2y3 tyt2y3 left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution, find_both_related is a good name.

@tyt2y3 tyt2y3 requested a review from billy1624 May 20, 2023 14:48
@Bolognafingers
Copy link

Shame this wasn't merged

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.

find_also_related() with non null relation, or using inner join, should not return Option
4 participants