Allow Launcher to set Factory. Fixes #862. #863
Conversation
tdurieux
changed the title
|
Immutable objects are better. Could you do what you want to do with a new constructor "Launcher(Factory f)" |
Indeed.
Yes. Will you add this constructor or should I add another PR? |
|
The best thing is to force-push a new commit on this PR/branch. |
done. |
| if (pFactory == null) { | ||
| throw new IllegalArgumentException("unable to create launcher with null factory"); | ||
| } | ||
| factory = pFactory; |
There was a problem hiding this comment.
Is it voluntary that processArguments is not call in this constructor?
There was a problem hiding this comment.
You can also call this constructor from the previous one this(createFactory()) in order to reduce code duplication.
There was a problem hiding this comment.
I tried but Java (for good reasons) does not allow to call none static methods before this has been called. The actual compiler message is:
cannot reference this before supertype constructor has been calledThere was a problem hiding this comment.
This constructor will work.
public Launcher() {
this(createFactory());
}There was a problem hiding this comment.
For me it doesn't. I tried both, OracleJDK (8) and OpenJDK (8).
There was a problem hiding this comment.
My bad, createFactory is not static...
This one works but it is hugly.
public Launcher() {
this(new FactoryImpl(new DefaultCoreFactory(), new StandardEnvironment()));
}There was a problem hiding this comment.
I could add another static function to Launcher, or leave it as is (or use your approach). Either one is fine.
There was a problem hiding this comment.
It is ok for my to keep it this way.
No description provided.