add static complete method that takes partial and returns full config
#54
Conversation
| @@ -35,6 +35,23 @@ public static ServiceConfigurationFactory of(ServicesConfigBlock services) { | |||
| return new ServiceConfigurationFactory(services); | |||
| } | |||
|
|
|||
| public static ServiceConfiguration create(PartialServiceConfiguration partial) { | |||
There was a problem hiding this comment.
it seems like a ServiceConfigurationFactory#create method ought to return a ServiceConfigurationFactory, rather than a ServiceConfiguration.
There was a problem hiding this comment.
Any suggestions on the method name? since it is a factory class, i thought using create to return a ServiceConfiguration would be appropriate. maybe be get?
|
Fixes #32 |
qinfchen
changed the title
complete method that takes partial and returns full config
| @@ -35,6 +35,24 @@ public static ServiceConfigurationFactory of(ServicesConfigBlock services) { | |||
| return new ServiceConfigurationFactory(services); | |||
| } | |||
|
|
|||
| /** returns a complete {@link ServiceConfiguration} for the given partial one. */ | |||
There was a problem hiding this comment.
add Javadoc / throws IllegalArgumentException to signal to caller when / how this can fail
@throws IllegalArgumentException when the given partial's security configuration is absent
|
any additional comments? |
| * | ||
| * @throws IllegalArgumentException when the given partial's security configuration is absent | ||
| */ | ||
| public static ServiceConfiguration complete(PartialServiceConfiguration partial) { |
There was a problem hiding this comment.
I'm a little scared of this method since it's very likely we're going to forget updating it when optional fields are added to ServiceConfiguration. Absent a proposal for making this compile-time or test-time safe, I'd prefer not taking this change at the expense of a little more consumer code (need to go through ServiceConfigBlock).
There was a problem hiding this comment.
Good point. The current propagateDefaults method would have the same concern as mentioned. Since there is no compile-time or test-time safe, the consumer code will be prone to the same mistake, no?. Since partial and complete configurations only differ by the optionality of one property, would a common interface between them be better?
There was a problem hiding this comment.
ping? I can help with whatever proposal we can come up.
There was a problem hiding this comment.
Yes, the propagate method has the same problem, but at least it's only one place rather than two. We could remove or deprecate the builder and use full constructors, at least internally.
There was a problem hiding this comment.
That works. Could the pattern like below (from the Immutables doc) be another solution?
interface Vehicle {
}
interface VehicleBuilder {
// Generated builders will implement this method
// It is compatible with signature of generated builder methods where
// return type is narrowed to Scooter or Automobile
Vehicle build();
}
@Value.Immutable
public abstract class Scooter implements Vehicle {
public abstract static class Builder implements VehicleBuilder {}
}
@Value.Immutable
public abstract class Automobile implements Vehicle {
public abstract static class Builder implements VehicleBuilder {}
}
class Builders {
void buildIt(VehicleBuilder builder) {
Vehicle vehicle = builder.build();
}
void use() {
buildIt(ImmutableScooter.builder());
buildIt(ImmutableAutomobile.builder());
}
}
|
Have to check how this works with Jackson annotations and serialization
|
No description provided.