[#705] Configuration phase two #766
Conversation
dmlloyd
commented
Feb 6, 2019
dmlloyd
commented
- Configuration keys are now derived from the config root class name by default (not injection site)
- Introduce new enum constants for each valid configuration phase
- Configuration keys are now derived from the config root class name by default (not injection site) - Introduce new enum constants for each valid configuration phase
| */ | ||
| STATIC_INIT, | ||
| BUILD_AND_RUN_TIME_FIXED(true, true, false, false), |
There was a problem hiding this comment.
This is a minor comment but we usually write runtime in one word in the existing code. See ExecutionTime for instance.
Not sure we would want it to be written differently.
There was a problem hiding this comment.
Yeah but then you get BUILDTIME_AND_RUNTIME_FIXED and it seems a bit silly. To me "run time" is a time and a "runtime" is a thing that does stuff.
Put another way, "runtime" is a word, but "buildtime" is not a word. We'd say "augment time", "install time", "debug time" etc.
There was a problem hiding this comment.
Well, not preferring one of the others, but I thought we should try to be consistent as IIUC we're talking about the exact same thing in the 2 classes (and we will probably use them in the same Processor classes).
@stuartwdouglas WDYT?
| @@ -36,7 +36,7 @@ | |||
| * @param diagnostics the diagnostics associated with the build failure (not {@code null}) | |||
| */ | |||
| public BuildException(final List<Diagnostic> diagnostics) { | |||
| Assert.checkNotNullParam("diagnostics", diagnostics); | |||
| super(constructMessage(null, Assert.checkNotNullParam("diagnostics", diagnostics))); | |||
There was a problem hiding this comment.
I would have put the assertion in constructMessage. It would be a bit more readable IMHO.
There was a problem hiding this comment.
I suppose it's a matter of preference. The method was designed to be used exactly like this though.
| private final ConfigPatternMap<LeafConfigType> leafPatterns = new ConfigPatternMap<>(); | ||
| private final IdentityHashMap<Object, ValueInfo> realizedInstances = new IdentityHashMap<>(); | ||
| private final HashMap<String, RootInfo> rootTypes = new HashMap<>(); | ||
| private final TreeMap<String, RootInfo> rootTypesByContainingName = new TreeMap<>(); | ||
| private final TreeMap<String, RootInfo> rootTypesByKey = new TreeMap<>(); |
There was a problem hiding this comment.
So, let's ask the big question: I remember us asking you if it would be possible to share the same root and have build time and runtime configuration properties in it.
Typically having:
shamrock.hibernate-orm.my-build-time-property =
shamrock.hibernate-orm.my-runtime-property=
Is it possible with your model? The classes would need to be different (and I suppose I would like to be able to define a name in @ConfigRoot as there's a good chance I would want a different names for the root) but I wonder if you don't end up having issues with these very maps. I haven't looked all the details but I thought I might ask.
There was a problem hiding this comment.
It is, after this patch. You would have to share the same config root class though, there's no duplication of keys allowed (as this causes ambiguity in parsing). But yeah all the config classes can be shared now. But I still think that build items is a better way to cooperate between extensions.
There was a problem hiding this comment.
In order to have build and run time properties in the same root key, which after re-reading this is what you really seem interested in, you must have two separate non-overlapping root classes. Any configuration property can be "anonymous" i.e. inherit the parent's name, so that's how you'd do it: two config roots with the same name = attribute, each with a different phase and each containing an anonymous configuration group.
There was a problem hiding this comment.
It's not tested though, so whoever is the first person to try it will get to report the bugs :)
There was a problem hiding this comment.
two config roots with the same
name =attribute
Ah missed the new added parameter.
|
OK, merging as I suspect it will conflict with a lot of our current work. Let's discuss the naming in a separate issue. |
