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(repository): created hasAndBelongsToMany following convention pa… #2315

Conversation

clayrisser
Copy link
Contributor

@clayrisser clayrisser commented Jan 31, 2019

I created some boilerplate for the hasAndBelongsToMany relation using the hasMany relation. Currently this contains the identical functionality as the hasMany relation.

See also #2308

Checklist

  • 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

@TomerSalton
Copy link

TomerSalton commented Jan 31, 2019

hey @codejamninja ,
Since we are both assigned on issue #2308 , I would like us to cooperate.
Let's avoid doing duplicated work and try to speed up the development process.

I'm currently working on the design and on the repository interface.
Can we agree to share our progress and knowledge in here?

@clayrisser
Copy link
Contributor Author

@TomerSalton, Absolutely! Maybe we should sync up over a call sometime too.

@bajtos bajtos added the Relations Model relations (has many, etc.) label Mar 1, 2019
@dougal83
Copy link
Contributor

dougal83 commented May 6, 2019

up

@guitarhero1 Hey, please don't make meaningless posts that contribute nothing to the conversation. If you are attempting to "bump" the topic, please consider simply registering interest using the reaction feature. All you've done here is add a pointless notification to everyone watching the thread(as have I). This code will likely be reviewed as workloads allow, not on demand.

Apologies for the extra notification form this post.

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.

@codejamninja I think it would be easier for all if hasAndBelongsToMany is implemented in smaller incremental steps. This pull request is a good start, I am proposing to make few changes and land it, so that other work can be based on the interfaces implemented here.

My main concern is whether getTargetRepository + constraint is sufficient to build HasAndBelongsToManyRepository. Is there a way how to leave that decision out of this initial pull request? For example, we can leave DefaultHasAndBelongsToManyRepository out of this pull request, but I am not sure if there will be enough meaningful code left after that change? Alternatively, maybe we can change the repository class to throw "Not implemented" error for now and clearly say in the class tsdoc that the API is experimental and may change within semver-minor releases.

Thoughts?

/cc @raymondfeng

@@ -210,6 +213,71 @@ export class DefaultCrudRepository<T extends Entity, ID>
);
}

/**
* @deprecated
Copy link
Member

Choose a reason for hiding this comment

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

Let's not add methods that are already deprecated at creation time. Please remove _createHasAndBelongsToManyRepositoryFactoryFor from the pull request.

* the target repository instance
*/
constructor(
public getTargetRepository: Getter<TargetRepository>,
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if it's enough to provide target repository getter, don't we also need getter for the intermediate ("through") repository?

@raymondfeng
Copy link
Contributor

@codejamninja Thank you so much for the efforts and patience. I'll find some time to review it tomorrow.

@clayrisser
Copy link
Contributor Author

@raymondfeng no worries and thanks. If you wouldn't mind, review the hasManyThrough using hasMany first. This pull request cannot be finished until hasManyThrough is accepted.

#2359

@dhmlau
Copy link
Member

dhmlau commented Aug 19, 2020

We just switch the contribution method from CLA to DCO, making your contribution easier in the future. Please sign the commits with DCO by amending your commit messages with -s flag and push the changes again. If you're using the command line, you can:

git commit --amend -s
git push --force-with-lease

Please refer to this docs page for details. Thanks!

@bajtos bajtos added the stale label Aug 20, 2020
@stale
Copy link

stale bot commented Sep 20, 2020

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this Sep 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked major Relations Model relations (has many, etc.) stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants