Create the ConfigBuilder #196
Conversation
|
Hi. So how do we move forward here? I guess this and #180 overlaps too much to keep both open, right? I'd close #180 therefore and continue with this PR here (not only, but also because I want to encourage you to contribute! 😆 ). If you don't mind, feel free to take patches from git.beyermatthias.de/config - branch 'szarykott-builder-patches' ( |
|
Thank you for the heads up @matthiasbeyer ! Correct, those two PRs follow the same principle with minor differences. Thank you for willingness to merge my PR then. I'll see what are the changes you propose and push them here. I will handle it, but it might take me few days. |
|
No worries, we are not in a hurry! 😆 |
|
I merged your patches and pushed them. Could you please resolve all pull request discussions you consider solved for better visibility of what is there to do? If you are ok with the way it is, I am also going to refactor tests to use builder pattern, due to a large number of deprecation warnings in tests. |
|
I tried to squash commits, something weird happened, please be patient ;) |
|
I resolved everything and I will do a new review once you're ready! 😄 |
|
Hello again! I think this is more-less the final version of this pull request. There are a lot of files changed, but most of them are tests. Here's a brief description of the changes.
|
There was a problem hiding this comment.
Awesome.
One comment, but that wouldn't block merging. I rather merge now and do some more changes later 😃
One more rebase please:
- reword the first commit to just "Add ConfigBuilder for builder-pattern style Config initialization"
- squash into first commit: b2e50ff
- squash 43973fe and fc0b227 into 6d2dada, rewording to "Use builder-pattern in tests, keep old interface tests in legacy" (or something like that)
- drop de097d3
You can keep the other commits, especially 2f8aca7 (but, if possible, with some more appropriate wording) - because it can easily be reverted later.
Sorry for another round of rebasing, I hope you don't mind. Otherwise, this is ready for merging IMO.
src/builder.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// Fails if `Expression::from_str(key)` fails. | ||
| pub fn set_default<S, T>(&mut self, key: S, value: T) -> error::Result<&mut ConfigBuilder> |
There was a problem hiding this comment.
I'm thinking about why we're using the &mut self builder-pattern style here. Maybe mut self would be more appropriate (moving and returning Self), so we can chain the build() function properly?
Nothing that blocks this PR from landing though!
That would also make the impl Source for [Box<dyn Source + Send + Sync>] unnecessary I guess.
There was a problem hiding this comment.
Actually, I've used &mut self, because this is how it was used in Config.
But the point is valid, the consuming version would allow chaining things properly, I am going to change this PR. After all, if it turns out in the future that the &mut self version is required by users, it is "less breaking" to lessen required ownership (self -> &self, etc.) than the other way around.
It would not make this implementation you mention unnecessary as it is needed because there is a build_cloned function that takes &self. It makes sense to have a cloned version, especially when speaking about the configuration that can change over time (etcd, consul, even files in rare circumstances) that you might want to refresh periodically in your application.
There was a problem hiding this comment.
files in rare circumstances
not even in rare ones: Think of a server software which should be able to pick up config changes without restarting!
I am going to change this PR
Awesome!
There was a problem hiding this comment.
I am happy you agree about the two flavors of the build method. I did not want to sound too pushy on this as I remember your opposition to the idea in some earlier comments, but maybe I just recall wrongly.
Code is changed, ConfigBuilder's methods are not consuming, except for one of two build methods. Feel free to merge as soon as you want!
There was a problem hiding this comment.
I did not change commit messages exactly as you wanted, but I hope it is fine the way it is.
szarykott
force-pushed
the
builder
branch
2 times, most recently
from
597dbfb
to
da0b047
Compare
|
Yes @matthiasbeyer I'd like you to merge, I only need to resolve conficts. |
|
@matthiasbeyer merge when you want. I rebased my code on master and added one commit which modifies env.rs file in similar way that previous code did to other tests files (use builder in main tests and keep old tests in legacy_tests). |
|
Awesome. I really love the progress in this crate lately and the many people contributing! Thank you! |
This is a draft PR to show an idea of how could ConfigBuilder look like.
It does not bring a lot of new functionality. It just splits Config that was a mutable configuration combined with a builder into two separate structs. It brings certain benefits:
Immutability is considered good programming practice, but not everyone might like it. I'd like to hear the opinions of users of the library - do you mutate config often after application initial bootstrap, do you consider such a mutability a needed feature?
I do not intend to push it in its current form, it is just to showcase an idea. To consider it mergeable discussion has to take place regarding few subjects:
In the current state there is a lot of deprecation warnings, so please do not mind them now and focus on more high-level aspects of this PR.
I attach a graph of the intended workflow so that it is easier to see the big picture.
