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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(repository): add interface for hasManyThrough repository #4312

Merged

Conversation

KalleV
Copy link
Contributor

@KalleV KalleV commented Dec 16, 2019

The hasManyThrough repository interface defines the actions that can be performed on a many-to-many model relation.

This PR is a continuation of #2359 and implements a step from #2359 (comment).

Relates to: #4247

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • 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 馃憟

@derdeka derdeka added community-contribution Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package labels Dec 16, 2019
@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch 4 times, most recently from 6a2fce6 to 5dec57c Compare December 17, 2019 16:54
@KalleV KalleV requested a review from derdeka December 17, 2019 16:54
@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch from 5dec57c to 0f3ea26 Compare December 17, 2019 17:03
@KalleV
Copy link
Contributor Author

KalleV commented Dec 27, 2019

Hi. Are there any changes you would like to see to the interface?

@bajtos bajtos self-assigned this Jan 6, 2020
Copy link
Member

@bajtos bajtos 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 @KalleV for the contribution, I'd like to discuss the proposed Repository API design - see my comments below.

@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch 3 times, most recently from 42224cb to 1495269 Compare January 6, 2020 17:01
@KalleV KalleV requested a review from bajtos January 6, 2020 17:02
@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch from 1495269 to ea77f8b Compare January 6, 2020 17:04
@KalleV
Copy link
Contributor Author

KalleV commented Jan 6, 2020

@bajtos Thank you for the review. I addressed the comments. Let me know if you would like any further changes.

Copy link
Member

@bajtos bajtos 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 @KalleV for the update.

I would like to simplify the first version of this repository interface and make it more consistent with other relation repositories. This may be controversial, so let's wait for feedback from @raymondfeng @jannyHou @agnes512 and @nabdelgadir before proceeding further.

@strongloop/loopback-maintainers @strongloop/loopback-next may want to chime in too.

@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch 2 times, most recently from b64af60 to 95d1dd1 Compare January 8, 2020 15:16
@KalleV
Copy link
Contributor Author

KalleV commented Jan 8, 2020

@bajtos Thank you for the 2nd review. I simplified the interface as suggested and added the throughData property to the link method options.

@KalleV KalleV requested a review from bajtos January 8, 2020 15:18
Copy link
Contributor

@derdeka derdeka left a comment

Choose a reason for hiding this comment

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

Great, looks good to me.

I really like the idea of providing throughData in link. But i think there is also the usecase of updating this throughData. How to handle this?

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.

LGTM 馃憤

@KalleV
Copy link
Contributor Author

KalleV commented Jan 10, 2020

Great, looks good to me.

I really like the idea of providing throughData in link. But i think there is also the usecase of updating this throughData. How to handle this?

@derdeka Perhaps the patch method could also pass throughData which would optionally update the through model in addition to the target?

@derdeka
Copy link
Contributor

derdeka commented Jan 10, 2020

@derdeka Perhaps the patch method could also pass throughData which would optionally update the through model in addition to the target?

Let鈥榮 do that in later iterations.

@bajtos as assignee PTAL. Ready to get merged?

@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch 2 times, most recently from 192f60e to d41be0c Compare January 14, 2020 09:57
@hacksparrow
Copy link
Contributor

  • New tests added or existing tests modified to cover all changes

We need the corresponding tests.

@bajtos
Copy link
Member

bajtos commented Jan 14, 2020

We need the corresponding tests.

How do you propose to test this new interface? AFAIK, we don't have any tests for other relation-repository interface either.

From the perspective of testing, it would be better to land this interface together with an implementation, so that we have some coverage. On the other hand, since the new interface isn't used anywhere yet, I think it's ok to tweak or change it if we discover any problems in future pull requests.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

LGTM.

Please add "EXPERIMENTAL" warning to the API docs, just to be sure.

The hasManyThrough repository interface defines the actions that can be performed on a many-to-many model relation.

This PR is a continuation of loopbackio#2359 and implements a step from loopbackio#2359 (comment).
@KalleV KalleV force-pushed the kv_add_has_many_through_repository_interface branch from d41be0c to 9336c1e Compare January 14, 2020 15:42
@derdeka derdeka merged commit a242785 into loopbackio:master Jan 14, 2020
@KalleV KalleV deleted the kv_add_has_many_through_repository_interface branch January 15, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Relations Model relations (has many, etc.) Repository Issues related to @loopback/repository package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants