Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

global example: Use Config::builder() for building global settings #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

matthiasbeyer
Copy link
Collaborator

@danieleades your review would be appreciated (submitted seperately instead of adding it to #262)

Copy link
Contributor

@danieleades danieleades left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think this example is supported without using deprecated methods. Either

  • the updated example should be accepted with reduced scope/applicability to the original
  • the example should be removed entirely
  • the deprecation of the Config::set method should be revisited

Comment on lines -15 to -14
// Set property
SETTINGS.write()?.set("property", 42)?;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think these two versions are equivalent. the existing example is mutable global configuration, the second is immutable global configuration

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, AFAICS, my change is equivalent, because Config::set sets a default internally, which is now done via ConfigBuilder::set_default(). Mutability is still provided, as the resulting Config object is still in a RwLock.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, mutability on the Config object is not needed anymore, because there are no (non-deprecated) methods that take &mut self!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that's correct? Looks to me like Config::set sets an override, not a default. Thus it can be used for modifying the config after it's been parsed (as is done in the example).

Without using deprecated methods, it's not possible to modify the Config after its construction, since as you say, there are no methods that take &mut self. That also makes the RwLock redundant.

As far as I can tell, before the deprecation it was possible to modify global configuration programatically after its construction, and after deprecating the methods it is not. It could well be that this was intentional, but it does mean that some use-cases (including one this example demonstrates) are no longer possible

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Yes, you're right. Lets see what @szarykott says, but I guess we should re-think the deprecation (as you said)! 👍

@matthiasbeyer
Copy link
Collaborator Author

@szarykott I guess your input on this matter would be valuable!

alerque added a commit to alerque/casile that referenced this pull request Mar 3, 2022
@alerque
Copy link
Contributor

alerque commented Mar 3, 2022

I just tried to bump this crate in a project of mine and got completely hung up. I eventually came around looking for examples and eventually found this PR. I'm going to back off and just hold it back a version for now because it looks like my entire use case is dependent on something that went away.

@matthiasbeyer
Copy link
Collaborator Author

@alerque I am sorry to read that we broke your usecase! Is your code open source and could you point us to it, so we can understand what we broke and ideally how we can fix our side to make your experience good again?

alerque added a commit to alerque/casile that referenced this pull request Mar 3, 2022
@alerque
Copy link
Contributor

alerque commented Mar 3, 2022

Thanks for the reply @matthiasbeyer. I have several open source and some more private projects that I've used the same general paradigm for. It's possible I've just over complicated things by not understanding the other possible ways to set this up, but here is one example of what I'm doing:

https://github.com/sile-typesetter/casile/blob/master/src/config.rs

The resulting mutable state CONF struct is used to setup runtime config values from a hierarchy: defaults → system config file → user config file → project config file → environment vars → cli args → runtime adaptations (e.g. a subprocess giving output that asks for a mode to be enabled for future subprocesses). One example of a few of those things being done:

https::/github.com/sile-typesetter/casile/blob/master/src/main.rs#L9-L15

Other places CONF state may be accessed or set in threads (hence the sync::RwLock wrapper).

If there is a way to code this that can handle the multi-threaded access with a set of defaults and the ongoing mutability through the life cycle then I'm open to adapting.

@matthiasbeyer
Copy link
Collaborator Author

I don't see why you would have your configuration in a global variable? Having it initialized in main and passed around would work, right?

@alerque
Copy link
Contributor

alerque commented Mar 4, 2022

No. That's what I started out trying to do, but besides being a royal pain to pass around all the time, I couldn't find a way to do so that allowed the configuration to be mutable from threads (rayon) that didn't run into lifetime issues.

@matthiasbeyer
Copy link
Collaborator Author

Hm. What were you trying to pass around exactly? An Arc<RwLock<Configuration>> kind of thing?

@blagovestb2c2
Copy link

blagovestb2c2 commented Mar 23, 2022

Hello, we are trying to bump the version to 0.12 but our case doesn't seem to be easy to port. What we did before was more or less:

let mut config = Config::default();

try merging with a File or ignore
try merging with another File or ignore

return config

the "try merging part" is very clunky:

let mut tmp = Config::default();
if tmp.merge(file source).is_err() { ignore ... }
else {
  config.merge(file source); 
}

I tried porting this to 0.12 and while doable, the lack of ability to mark certain sources as optional is making life hard. Furthermore, the lack of integration with std is making the whole process more complicated than it should be. For instance, why not implement Source for Option<Source> where you just consider the None case to be an empty configuration. Then we'd not have to break up call chains just to check if something is None. Moreover, being able to use HashMap<String, String> or similar common types as Source would be amazing. Alas, this is not implemented and it's not possible for users to implement it either, the Source trait being foreign and std being std...

The Config object once constructed is very useful, but getting it constructed can be a pain.

p.s. where did get_str() go :(

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

Rebased and fixed the formatting issue here. If we still want this, CI should be green now.

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.

None yet

4 participants