Implement ConfigProvider.Flat#merge #7700
Conversation
| def flatten: ConfigProvider.Flat = | ||
| new ConfigProvider.Flat { | ||
| def load[A](path: Chunk[String], config: Config.Primitive[A])(implicit trace: Trace): IO[Config.Error, Chunk[A]] = | ||
| ZIO.die(new NotImplementedError("ConfigProvider#flatten")) |
There was a problem hiding this comment.
Maybe it would be better to return Option[ConfigProvider.Flat] than die? I'm not sure.
There was a problem hiding this comment.
I think it depends on whether we think other ConfigProvider instances we define will be able to implement flatten.
With the changes in this PR every ConfigProvider constructor and every ConfigProvider operator returns a ConfigProvider that implements flatten.
If we can continue this in ZIO Config then I think this failure really is an extraordinary situation and die provides a smoother interface where users of flatten don't have to handle the possibility of None.
On the other hand, if we think that we will have a significant number of ConfigProvider constructors that can't implement flatten then I think we would need to look at alternatives.
| trace: Trace | ||
| ): IO[Config.Error, Chunk[A]] = | ||
| self.load(path, config).catchAll(e1 => that.load(path, config).catchAll(e2 => ZIO.fail(e1 || e2))) | ||
| def enumerateChildren(path: Chunk[String])(implicit trace: Trace): IO[Config.Error, Set[String]] = |
There was a problem hiding this comment.
Really clean. Feels like merge wants to be implemented at this level. 👍
|
Do we also want to change the default provider to use |
|
|
| new ConfigProvider { | ||
| def load[A](config: Config[A])(implicit trace: Trace): IO[Config.Error, A] = | ||
| self.load(config).orElse(that.load(config)) | ||
| ConfigProvider.fromFlat(self.flatten.merge(that.flatten)) |
There was a problem hiding this comment.
Are we sure we want to change the semantic of orElse versus just introducing merge? In some cases you may want to say, "Load this thing entirely, if you happen to randomly load some part of it, that's not valid, ignore it and try this other source instead." That's quite different than merge.
There was a problem hiding this comment.
It is a good question.
My thought was that applying the "or else" logic to the values in the ConfigProvider was significantly more intuitive. This issue originally arose because in doing something like Config.string("key").optional and envProvider.orElse(propsProvider) it was counterintuitive that this would return None if there was no value in envProvider but there was in propsProvider.
That was potentially due in part to orElse currently being the only binary operator to compose ConfigProvider values but I think this does still seem potentially surprising. Related to this, I thought there was value in having one way of doing things unless we thought both were really valuable.
One further issue is that I'm not sure the old orElse semantics make sense in the world in which every ConfigProvider has an underlying Flat implementation. Flat splits enumerating the keys and loading a value into separate operators. But the old orElse semantics imply that we need to load all the values in the left provider and make sure they can be loaded successfully before knowing whether we should enumerate the keys in the left provider or the keys in the right provider.
There was a problem hiding this comment.
All right, I think I'm open to that. The final question is: should we even introduce merge since it's another name for orElse?
There was a problem hiding this comment.
I think that makes sense, especially since this way of "merging" is left biased whereas normal merging of key value pairs would normally be right biased if you had to pick one.
Resolves #7624.