Skip to content

Conversation

@matthiasbeyer
Copy link
Member

This patch adds Config::with_sources() for builder-pattern multi-merge
without calling Config::refresh().
The documentation added in the patch serves as rationale.


I'd love to see discussion on this.

@szarykott
Copy link
Contributor

As discussed in #148 this kind of code could have the desired effect. It would be easy to extend with asynchronous configuration sources if they are ever implemented just by adding merge_async or similar methods.

I am thinking, however, that it might be cleaner to implement a separate struct called, for instance, ConfigBuilder that would produce a frozen config at the end that would be an entry point by itself, without a need to go through Config. Having such a separate struct would indicate clearly that workflow is different. On the other hand discoverability of such a feature would be smaller than in the case of a single entry point as it is a case with a single Config struct with methods on it.

Just a thought of mine on this matter.

@matthiasbeyer matthiasbeyer marked this pull request as ready for review March 26, 2021 17:03
@matthiasbeyer
Copy link
Member Author

So what'd you think? Merge? 😄

@matthiasbeyer
Copy link
Member Author

matthiasbeyer commented Mar 31, 2021

I will merge after Fri 2nd April 2021 12:00:00 UTC if there are no complains.

--

Ah, I will implement the suggested changes first, tho.

@matthiasbeyer matthiasbeyer added this to the 0.12.0 milestone Mar 31, 2021
This patch adds Config::with_sources() for builder-pattern multi-merge
without calling Config::refresh().
The documentation added in the patch serves as rationale.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer
Copy link
Member Author

So this is a bit off-topic for this branch, but anyways: The latest commit (which really should be squashed if we merge this) implements a builder-pattern entry point as you @szarykott suggested.

I kept the entry point for the builder on the Config type with Config::builder(), because that keeps the visibility of the whole thing. I did not (yet) remove the functions on the Config type itself, as I think we should rather deprecate them in one release and remove them in the following one. Changing all interface in one release seems to be a bit too big of a change for our users, IMO.

What do you think?

@matthiasbeyer matthiasbeyer changed the title Add Config::with_sources() ConfigBuilder (was: Add Config::with_sources()) Apr 2, 2021
@szarykott
Copy link
Contributor

szarykott commented Apr 3, 2021

I'd consider moving ConfigBuilder to a separate file as it seems a functionality of its own.

As you might have seen in #196 I'd like to separate Config and ConfigBuilder, which basically means having two structs - one with cache, read-only, and the other one having all mutable stuff inside. Right now they are kinda mixed as Builder just spawns a mutable Config underneath and uses its mutable methods.

Besides it might be better to merge #190 before considering merging this (due to ConfigKind::Frozen being removed).

@matthiasbeyer matthiasbeyer deleted the multi-merge branch April 19, 2021 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants