-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Traits #145
Traits #145
Conversation
https://circleci.com/gh/testcontainers/testcontainers-java/569 tests are green, shade test is failing because of visible assertions |
* Default settings | ||
*/ | ||
@NonNull | ||
private List<Integer> exposedPorts = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
each trait has it's own state, not listed here
This looks good - I've raised some comments but mainly these are just queries. As for merging this I think we're just about into breaking-change territory, unfortunately. I'm definitely not against jumping up to v2 in the near future, though. I think we just need to pick and choose which other things need to be closed off in 1.0.x, and which other things join this in v2. #87 is probably a prime candidate as it too will have some breaking API changes. What do you think? |
@rnorth thanks for the feedback! I agree that 2.0 is a better version for such feature because the API layer was dramatically changed, even if public changes are not that huge. BTW I'll try to research some ways how to workaround that nasty I'm concerned a little bit that this branch might become outdated, but will try to keep it up-to-date (and will start right now because there are some conflicts :) ) |
… it possible to extend it with composition, not inheritance
@rnorth merged with master |
Hi Guys, I've been reading through this pull request and here are my 2 cents:
Maybe we can try a bit simpler approach which won't be a breaking change immediately and can be adopted slowly and gain the composition approach.
I'm not sure but maybe this handles the extra <> you had to add to a What do you think? |
Hey @asafm, Thanks for the feedback! If I understood @rnorth correctly, he wants to keep "withSomethingSomething(...)" notation, so we can't just mark them deprecated. For me, generic SelfReference was added in #144 to make it possible to extend GenericContainer without losing fluent style of Of course, best option would be https://projectlombok.org/features/experimental/ExtensionMethod.html , so we could implement support methods ( |
Thanks @asafm. I'm very keen that we keep the library as usable as possible, so if there are concerns over ease of understanding I'd like to be careful to make sure we've considered all the options to simplify. Your suggestions are useful ideas. If you both wouldn't mind, I'd like to take a bit of time to mull things over and come back with a few proposals - probably skeleton class models rather than full fledged PRs. I'm afraid I'm going to be pretty much offline until next Tuesday, but hopefully will have a bit of time to think about this. I hope this is OK, and sorry for the delay in responding |
I've not stopped thinking about this one, but TBH I'm afraid this has been slightly pushed down my list by other things outside of the project plus the quite pressing need to equip TC for the 'native' Docker tooling before the end of that beta. That's going to be my main focus for a little while. Re this PR, I think I'd like to try and simplify a bit - I am a little worried that there's a degree of sophistication which could make this less accessible / more intimidating to users. The kinds of things I'm contemplating, though not necessarily precise, are:
I hope this is OK for now - I think I'll probably have to return to this topic after native docker support is a bit less of a worry, but thought I should note down my current thinking. |
@rnorth sure, if you'ill figure out how to simplify it, but keep composition - I'm all in :) |
I'll close this PR and start a new one, with less magical API |
Depends on #144 , but could be merged instead since it's included.
This pretty huge change makes it possible to use composition over inheritance in TestContainers. All "withXXXX" code was extracted from GenericContainer to the separate traits.
Unfortunately, because of Java limitations, it breaks user's code a little bit - they have to use
new GenericContainer<>(...)
now (note diamond generic notation).Motivation:
In our test system, we use "traits" to reuse some code to configure GenericContainer (currently, we have some base container which extends GenericContainer and adds support for a trait, but we want it in core).
For instance, we have a "recipe" to add the Java Agent, and we use it like:
Thanks to traits, we don't have to use inheritance to extend our containers - we just apply multiple traits to it.
@rnorth WDYT?