Config changes that may impact precedence order for overlapping config defined in multiple places. #1813
Conversation
This changes some semantics of config a bit... for example the order of precedence between aliases and other sources. Additional tests are needed for various providers, but ConfigTest should cover most cases.
| import lombok.extern.slf4j.Slf4j; | ||
|
|
||
| @Slf4j | ||
| final class ConfigConverter { |
There was a problem hiding this comment.
I wonder if type specific converters would be better than many static methods, but this is fine.
| try { | ||
| return PUBLIC_LOOKUP.findStatic(type, "valueOf", MethodType.methodType(type, String.class)); | ||
| } catch (final NoSuchMethodException | IllegalAccessException e) { | ||
| log.debug("Can't invoke or access 'valueOf': ", e); |
There was a problem hiding this comment.
I think this should by error. I also think this should fail immediately with an Exception or Error in tests.
There was a problem hiding this comment.
I can add an assert in ConfigProvider where the exception is being caught and ignored.
| } | ||
| } | ||
|
|
||
| public static ConfigProvider withPropertiesOverride(Properties properties) { |
There was a problem hiding this comment.
If ConfigProvider was a Composite of its own type rather than Sources, then this code would have less duplication.
There was a problem hiding this comment.
I don't understand what you mean here.
dougqh
left a comment
dougqh
left a comment
There was a problem hiding this comment.
I think there are few small things that could be improved further, but I think this is solid clean-up to Config. I'm mostly happy with this as is, but I still think it would be good to run these changes by the entire team.
Add assert for conversion exception.
|
|
||
| log.debug("New instance: {}", this); | ||
| } | ||
|
|
tylerbenson
changed the title
This changes some semantics of config a bit... for example the order of precedence between aliases and other sources.
The big benefit this has is removing the additional config constructor which was generally not used.
I think we could improve consistency between when we load config with the
dd.prefix and when we don't. I tried to maintain existing semantics when possible.