Implements ideas described in #8 and #9 #10
Conversation
… (handling of the defined DSL) and one responsible for the managing of the built objects in an EntityManager. New tests for the builder part implemented.
… used to compile groovy code
|
According to our discussion in #8 the commits above represent my view on how I would like to see the implementation of the library (not a WIP anymore :) ). Following has been done:
What could be done in addition:
|
|
Hi @dadrus, first of all, thanks for your efforts & your patience. As mentioned in #8 we were quite busy in the last months. Still, we hope to do better in the future. Phew! That's quite a lot of changes in the diffs. Breaking changes.The new package structure results in breaking API changes. This would force a major version change. While this is possible, we should take a moment to consider if this is necessary. TestDataLoaderFrom: We probably could avoid this one by just keeping the old EntityBuilder, Singleton (#8)In the data groovy scripts
offered code completion. With the PR this is not possible anymore, because a static import is no longer possible. I understand that the singleton that realizes the code completion is directly opposed to your requirements from #8. However, getting rid of it also gets rid of code completion. I'm also not quite sure about the use case of #8. Where would you need multiple Minor issues
#9There a so much changes, I fail to see which ones are necessary for #9. Where to go from hereSo, after spending several hours reviewing this, I must admit that the PR is just a bit too huge for me. Also, in general, you wrote a lot about what you changed but not always why. The why which is more important for me, because I cannot see it in the code. It's difficult to see from the commits to which issue they relate. I'm also convinced that you did a lot of changes that are not directly related to the two issues. I think it would be more transparent and easier to review if we split this PR in multiple smaller PRs that clearly show what was changed and why. I hope that that this does not scare you off. We're definitely interested in your work and want to enable other tools such as your dadrus/jpa-unit to use test-data-loader. |
|
Hi @schnatterer. Thank you very much also from my side for this extensive review. The enormous work you did is exactly what I wanted to avoid by initially defining this PR as WIP and by asking you to verify whether the direction I took in my initial commit is inline with your strategy for this project. I'll start my reply with more simple cases from your comments. Minor Issues:
Breaking changes
create Customer, "customer", {
name = "Foo"
account = create Account, "account", {
// whatever comes here
}
}create Customer, "customer", {
name = "Moo"
account = create Account, "account", {
// whatever comes here
}
}To be honest, I would even like to see an additional create method without a name argument. The name could be then generated internally, e.g as a UUID. For that change, One additional note regarding I hope, I could give you enough insights about the why. Regarding the PR split: the general PR for #8 will remain anyway a massive one. It will be same as today, but using your formatter rules (if I can use them in eclipse), without the package structure change and |
|
Hi @dadrus, thanks for your endurance and your detailed answer. Let's try to get us to a pragmatic merge. Most importantly, we're willing to get rid of the singleton. So let's make the breaking change from test-data-loader 0.x to 1.
As for the formatting, we've had a lot of discussions about formatting in different IDEs in the past. So our compromise is: Format the code you write code yourself (manually or with formatter). Don't use save actions, don't auto format complete files. So, how are we going to get this thing merged? Some final notes: |
…e rehydrated closure set to DELEGATE_FIRST
|
Hi @schnatterer, Cool. Let's try to approach it that way. In the meantime I implemented the dsld for eclipse, you can find in the above referenced commit. According to the documentation of both eclipse and IntelliJ, the dsld file just needs to be on the classpath to have code completion working. Thus it can reside in the resulting jar. The end user doesn't need to do anything. In case of eclipse the dsld file needs to reside in a special folder What about the following approach to make the review of the changes as easy as possible:
That way every commit will build on top of the other and you will have a meaningful history. Or do you rather like each of these topics become a separate PR? Regarding the |
|
Please take a look at following commits. If this type of history is ok for you, I'll create a new PR which will include them. |
|
Hi @dadrus, I had a short look at the I'll have to inspect da324f7 in detail, but I can do this as part of the new PR. So, yes please create a new PR (one is enough). I 'm glad to review (and hopefully merge) it by the end of next week. |
|
replaced by #12 |
WIP: This pull request is just to show how the implementation of this library can be enhanced to solve limitations/issues described in #8 and #9.
The main changes are:
EntityBuilderis not a singleton any moreEntityBuilderuses a new DSL object, which implements thecreatemethod, each time a new script file needs to be parsed and returns anEntityStoreobject, which holds all the references to the loaded objectsDataTestLoaderis adopted to the newEntityBuilderAPIOpen topics, respectively topics to be discussed (from my point of view) are:
EntityBuilderbecame immutable, thus it is a real builder now. But the object it returns, theEntityStore(a new introduced interface) is not really immutable. It defines aclearmethod, which is actually not required (the entire object can just be dropped instead of using this method. Adding of new objects is anyway not possible to it). The background of this method was to retain the previous functionality ofEntityBuilder.EntityBuilder). SinceDataTestLoaderis now responsible for handling multiple files, the corresponding functionality has to be implemented here.EntityBuildingExceptionandNoSuchElementExceptionare used for same purposes but in different methods. I would suggest to get rid ofEntityBuildingExceptionand useNoSuchElementExceptiononly..withDefault { ... }on map/list definition. Things like this would made the code more clear.What do you think?