Add fallback ConfigReferenceResolver #497
Conversation
| * value will cause the reference to be left unresolved in the config. | ||
| */ | ||
| public ConfigValue getReferenceValue(); | ||
|
|
There was a problem hiding this comment.
Does the resolver need to know about optional - it could return null for not found, and let the library throw an exception if not optional?
I feel like this interface could be replaced with parameters to the lookup method in ConfigReferenceResolver?
ConfigValue lookup(String path) ?
If we needed other parameters in the future we could add a variant of ConfigReferenceResolver perhaps.
There was a problem hiding this comment.
The reason I did it this way was mainly so that the reference value was available so there was a way for resolvers to express leaving the references unresolved. But I guess you can achieve a more coarse-grained version of the same thing by setting allowUnresolved=true and returning null.
| * Implement this interface to define how references that aren't defined within | ||
| * the config itself should be resolved. | ||
| */ | ||
| public interface ConfigReferenceResolver { |
There was a problem hiding this comment.
I believe the docs mostly say "substitution" and not "reference" but I could misremember. ConfigReference is an internal class I think called that for some historical reason. ConfigResolver might be fine even, keep it shorter.
| * otherwise resolved. Never returns null. | ||
| */ | ||
| public ConfigReferenceResolver getFallbackResolver() { | ||
| return this.resolver; |
There was a problem hiding this comment.
The method names here could follow the conventions for ConfigIncluder in parse options perhaps; would be appendResolver and getResolver I believe.
| */ | ||
| public Path getRemainder(); | ||
|
|
||
| } |
There was a problem hiding this comment.
I'd be inclined to let people use ConfigUtils.splitPath rather than making this public, since that's how the rest of the APIs have gone so far.
There was a problem hiding this comment.
Done, that simplifies things.
| return ResolveResult.make(newContext.removeCycleMarker(this), this); | ||
| else | ||
| throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString()); | ||
| throw new ConfigException.UnresolvedSubstitution(origin(), expr.toString()); |
There was a problem hiding this comment.
wasn't clear to me here why the code for checking getAllowUnresolved was removed; I see part of it is now in getFallbackSubst but not the removeCycleMarker
There was a problem hiding this comment.
I'm not sure I see the case where the behavior is different -- it should fall through to the else case where it does removeCycleMarker. Or maybe I'm following the control flow wrong? In any case I've become unsure whether the second call to getFallbackSubst is the right thing to do, should fallbacks be used if there are cycles? If not then getFallbackSubst only gets called once and I don't really need it.
| /** | ||
| * A reference resolver that never resolves anything. | ||
| */ | ||
| public final class NullConfigReferenceResolver implements ConfigReferenceResolver { |
There was a problem hiding this comment.
If this class is only used internal to ConfigResolveOptions would it make sense to make it a nested private static class inside ConfigResolveOptions?
There was a problem hiding this comment.
Done. I didn't see many inner classes so I wasn't sure what the preferred style was.
| */ | ||
| public ConfigResolveOptions appendResolver(ConfigResolver value) { | ||
| if (value == null) { | ||
| throw new NullPointerException("null resolver passed to appendResolver"); |
There was a problem hiding this comment.
this should be ConfigException.BugOrBroken probably, though I think a lot of the library doesn't bother to check null
| * added repeatedly. | ||
| * | ||
| * @param fallback the previous includer for chaining | ||
| * @return a new includer |
|
One thing I forgot to ask: I've added some "@SInCE x.x.x" but didn't know which version. Is there a version I can use or should I just drop them? |
|
using 1.3.2 probably has a good chance to be correct, thanks |
|
Done. Related question: is cutting 1.3.2 on the horizon? I ask because I have to decide if I should switch to building the latest from source to get this change or if I can wait a while and then just use the one from maven. |
|
@2m I saw you were triaging some bugs today; do you have time to make a release? the instructions are in CONTRIBUTING.md, though it may be that I'm the only one with the maven central permissions, we should probably fix that anyway... It's nice before a release to go through all the issues (as it appears you are) and also see if any PRs are unmerged that should have been merged, but probably a bad idea to block on work that we don't have time for, can always make another release. |
Add fallback ConfigReferenceResolver
Here is a suggestion for how to support fallback reference resolution. I'm interested in whether you would suggest any major changes -- like, is the options the right place to hold the resolver and does it make sense to create the path interface.