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.

Sign up for GitHub

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

Jump to bottom

Implement HasAndBelongsToMany relation #6452

Closed
wants to merge 1 commit into from
Closed

Implement HasAndBelongsToMany relation #6452

wants to merge 1 commit into from

Conversation

frbuceta
Copy link
Contributor

@frbuceta frbuceta commented Sep 28, 2020 •
edited
Loading

Implements #2308

81708637-a80f1f00-9471-11ea-93c7-269e002cb712

#2043 (comment)

Much of the code uses the same functions as HasManyThrough

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@dhmlau dhmlau added community-contribution Relations Model relations (has many, etc.) labels Sep 28, 2020
@dhmlau dhmlau added this to the Oct 2020 milestone Sep 30, 2020
@nabdelgadir
Copy link
Contributor

Hi @frbuceta, thanks for the PR! What's the current status?

@frbuceta
Copy link
Contributor Author

frbuceta commented Oct 6, 2020

I have little left now, I would just have to try it

@frbuceta frbuceta force-pushed the feat/has-and-belongs-to-many-relation branch 4 times, most recently from f94eca7 to a071cd7 Compare October 21, 2020 21:28
@frbuceta frbuceta force-pushed the feat/has-and-belongs-to-many-relation branch 4 times, most recently from dfff57b to f7338c5 Compare October 24, 2020 14:40
@frbuceta frbuceta marked this pull request as ready for review October 24, 2020 15:04
@frbuceta frbuceta requested a review from hacksparrow as a code owner October 24, 2020 15:04
@frbuceta
Copy link
Contributor Author

@strongloop/loopback-maintainers can you take a look at this?

@mrmodise
Copy link
Contributor

mrmodise commented Oct 24, 2020 •
edited
Loading

@frbuceta thanks for the PR. Please squash your commits. If it helps you can go through the submitting PR process: https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing

@frbuceta
Copy link
Contributor Author

@frbuceta thanks for the PR. Please squash your commits. If it helps you can go through the submitting PR process: https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing

Not all commits are the same but if I have to squash it, I squash it

@mrmodise
Copy link
Contributor

mrmodise commented Oct 25, 2020 •
edited
Loading

@frbuceta thanks for the PR. Please squash your commits. If it helps you can go through the submitting PR process: https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing

Not all commits are the same but if I have to squash it, I squash it

Thanks, have a look at this for more clarity: https://loopback.io/doc/en/lb4/submitting_a_pr.html#9-final-rebase-and-squashing-of-commits. I would squash all the commits to: feat: implement HasAndBelongsToMany relation since thats what the PR is addressing. Your thoughts?

@frbuceta frbuceta force-pushed the feat/has-and-belongs-to-many-relation branch from f7338c5 to 87d725a Compare October 26, 2020 10:34
@frbuceta
Copy link
Contributor Author

@frbuceta thanks for the PR. Please squash your commits. If it helps you can go through the submitting PR process: https://loopback.io/doc/en/lb4/submitting_a_pr.html#6-rebasing

Not all commits are the same but if I have to squash it, I squash it

Thanks, have a look at this for more clarity: https://loopback.io/doc/en/lb4/submitting_a_pr.html#9-final-rebase-and-squashing-of-commits. I would squash all the commits to: feat: implement HasAndBelongsToMany relation since thats what the PR is addressing. Your thoughts?

I have squashed all commits into one

agnes512
agnes512 reviewed Oct 28, 2020
View reviewed changes
Copy link
Contributor

@agnes512 agnes512 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 PR @frbuceta ! The PR does contain the code and tests the relation needs 👍

I think there are several parts that can be polished such as reusing the code from relation hasManyThrough as they are similar.

I like how you are improving some debug strings and file names 👍 to make the review process easier, it'd be great if you move them to another PR and maybe land it first.

packages/repository/src/relations/has-and-belongs-to-many/has-and-belongs-to-many.decorator.ts
RelationType,
} from '../..';

export function hasAndBelongsToMany<TH extends Entity, TA extends Entity>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having a decorator for every new relation, have you considered to move the through member to HasManyDefinition so that we can reuse the code and have better UX. Ref: #5354

packages/repository/src/relations/has-and-belongs-to-many/has-and-belongs-to-many.helpers.ts
HasAndBelongsToManyDefinition
>;

export function resolveHasAndBelongsToManyMetadata(
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that the code is the same/similar to has-many-through.helpers.ts. As these two relations are similar, maybe we can reuse the code to simplify it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, much of the code I use is the same as in HasManyThrough as I explain in the description of the PR. I will work to simplify it.

@dhmlau dhmlau modified the milestones: Oct 2020, Nov 2020 Oct 30, 2020
@dhmlau dhmlau removed this from the Nov 2020 milestone Dec 6, 2020
@frbuceta frbuceta marked this pull request as draft March 13, 2021 09:43
@frbuceta frbuceta force-pushed the feat/has-and-belongs-to-many-relation branch from 87d725a to 95c7434 Compare March 13, 2021 09:46
@stale
Copy link

stale bot commented Jul 14, 2021

This pull request has been marked stale because it has not seen activity within two months. It will be closed within 14 days of being stale unless there is new activity.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Jul 28, 2021

This pull request has been closed due to continued inactivity. If you are interested in finishing the proposed changes, then feel free to re-open this pull request or open a new one.

@stale stale bot closed this Jul 28, 2021
@frbuceta frbuceta deleted the feat/has-and-belongs-to-many-relation branch August 1, 2021 21:51
@albjeremias
Copy link
Contributor

what happened? why is this closed and not merged? Is there a workaround for the
HasAndBelongsToMany ?
maybe using a "belongsTo" and "hasMany" ?!

@welchsteven welchsteven mentioned this pull request Aug 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agnes512 agnes512 agnes512 left review comments

@nabdelgadir nabdelgadir Awaiting requested review from nabdelgadir nabdelgadir is a code owner

@hacksparrow hacksparrow Awaiting requested review from hacksparrow

Assignees

@nabdelgadir nabdelgadir

@agnes512 agnes512

Labels
Relations Model relations (has many, etc.) stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@frbuceta @nabdelgadir @mrmodise @albjeremias @agnes512 @bajtos @dhmlau