repository programming model #273
Conversation
implement the proposal in jakartaee#223
|
At first glance, there is a lot of good information here and lots of it looks good, but I highly recommend splitting this up and delivering it in smaller pieces so that we can ensure it is sufficiently well-reviewed. |
|
@njr-11 I don't want to split it up into bits because it's a single internally self-consistent model. The "bits" don't make sense in isolation from each other. And the core if this change is only 200 lines of contiguous text. So I would much prefer to take feedback as comments on the relevant lines. (Or as comments on issue #223, of course.) |
Co-authored-by: Nathan Rauh <[email protected]>
Co-authored-by: Nathan Rauh <[email protected]>
Co-authored-by: Nathan Rauh <[email protected]>
Thanks, and sorry for the unacceptably slow response time.
I think they're always available from the Conversations drop-down. |
There was a problem hiding this comment.
@gavinking I revised my review comments based on the decision that was made at this past week's Jakarta Data meeting. It seems like we reached agreement on most of what is covered in this pull, so hopefully once the updates from the review comments are included, we'll be able to merge in your pull.
Co-authored-by: Nathan Rauh <[email protected]>
Co-authored-by: Nathan Rauh <[email protected]>
Co-authored-by: Nathan Rauh <[email protected]>
Co-authored-by: Nathan Rauh <[email protected]>
|
@gavinking with Otavio's pull to add the lifecycle annotations now being merged, I was able to resolve one of the 4 remaining review comments. Could you incorporate the remaining 3 suggestions (I don't think these should be controversial) and rebase your pull to resolve merge conflicts? After that I think we should be ready to merge this. Thanks! |
|
We need to make progress on getting this issue in because it is accumulating merge conflicts with other pulls that update the main spec document. I'll look into merging in the suggestions, which I don't see any opposition to, resolve the current set of merge conflicts, switch the pull to Ready for Review, and approve my own review of the pull. If I'm able to do these things successfully, then I'll post a second comment to let others know to review it, and then hopefully we can get it merged. |
| A repository may declare lifecycle methods for a single entity type, or for multiple related entity types. | ||
| Similarly, a repository might have query methods which return different entity types. | ||
|
|
||
| A repository interface may inherit methods from a superinterface. |
There was a problem hiding this comment.
Could you explain more about this point?
What are you talking about here?
Are you talking about buil-in repositories? Or non-built-in ones?
There was a problem hiding this comment.
Are you talking about buil-in repositories? Or non-built-in ones?
Given that all the built-in repositories only allow for a single entity type, per the parameterized type variable for the entity, we can assume that Gavin is targeting these statements at user-defined (non-built-in) repository interfaces.
There was a problem hiding this comment.
Yes, that's right.
|
This is ready for review now if anyone else want to review it. I was able to successfully resolve the merge conflicts and add in the suggestions. |
Co-authored-by: Otávio Santana <[email protected]>
Co-authored-by: Otávio Santana <[email protected]>
| A repository may declare lifecycle methods for a single entity type, or for multiple related entity types. | ||
| Similarly, a repository might have query methods which return different entity types. | ||
|
|
||
| A repository interface may inherit methods from a superinterface. |
There was a problem hiding this comment.
| A repository interface may inherit methods from a superinterface. | |
| A non-built-in repository interface may inherit methods from a superinterface. |
Can we procedure this way?
WDYT?
There was a problem hiding this comment.
My vote would be against adding that language because it's also true that built-in repository interfaces can inherit methods from a superinterface. This currently exists in the spec where PageableRepository inherits from BasicRepository and inherits its methods as well. And where CrudRepository inherits from BasicRepository and inherits its methods as well.
If you feel strongly about it, then go ahead and commit the change, and merge the pull because this appears to be the last remaining review comment.
Co-authored-by: Otávio Santana <[email protected]>
Co-authored-by: Otávio Santana <[email protected]>
Co-authored-by: Otávio Santana <[email protected]>
|
We have a long time in this PR; I will merge it and open PRs to improve it. |
Apologies folks, and thanks for merging this. I came back today to resolve the conflicts, and fix up the remaining suggestions but I see I was too late. If you let me know about any concerns that were left unresolved, I will send a PR with those edits later today. @otaviojava @njr-11 |
Thanks for all of your contributions on this @gavinking ! I believe we resolved everything and am not aware of anything else to follow up on from this pull. |
|
OK thanks. Sorry again for the slow turnaround time. |
|
@gavinking thank you for all the support. Please, let us know if we are missing anything. |
|
@otaviojava yes I will do that, I promise! |
This draft implements the ideas proposed in #223.
This is unfinished, since it updates only the specification, and not the Java APIs.
I'm sending it "early" for the purpose of obtaining feedback.