rust-cli / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
https://docs.rs/config/latest/config/
Apache License 2.0
2.63k stars 216 forks source link

Expose ValueKind #62

Closed j-t-d closed 3 years ago

j-t-d commented 6 years ago

Would it be possible to expose ValueKind from value.rs to allow some introspection?

ie, change: pub use value::Value; in lib.rs to pub use value::{Value, ValueKind};

XX commented 6 years ago

I alse need access to ValueKind to implement serialization of my settings struct to config.

mehcode commented 6 years ago

I'll be removing ValueKind in 0.10 (and replacing it with just Value). Switching to new method of tracking errors (which was the reason for separation).

I'll keep this open until that happens on master.

greenwoodcm commented 4 years ago

It looks like ValueKind still exists on 0.10.1. I still need it because I want to match on the contained type and value of a Value. I was going to hack around it by invoking into_bool, into_int, etc one by one, but that seems to not work because those will coerce the value if possible. For instance, I can't distinguish between Boolean(true) and Integer(1).

Do you think it makes sense to just expose ValueKind?

gavinleroy commented 3 years ago

@mehcode I'm trying to implement the Source trait for an object and I'm unable to create a Value due to the privacy of ValueKind.

Could we expose ValueKind?

matthiasbeyer commented 3 years ago

Is this still desired in 0.11? I'm hesitant to make ValueKind public, actually.

polivbr commented 3 years ago

I'm looking to write a source that sits on top of a config and does variable interpolation, and having access to ValueKind would make this much easier, as I need to recurse through the tree of values.

matthiasbeyer commented 3 years ago

What does your approach look like? IMO you could load the config as a struct LoadedConfig and then do the interpolation from there LoadedConfig::interpolate(variables)? // returns Result<Config>, after the configuration object is loaded from config. For that you'd not need access to the ValueKind!

Or am I missing something?

polivbr commented 3 years ago

How do I recurse across the hierarchy of values in the Config, performing interpolation on the string values, regardless of how deep they are in the hierarchy? If I had access to ValueKind, I can match on the type, processing strings, ignoring other scalar types, and recursively process arrays and maps. Not sure how I would handle this without ValueKind.

Can I ask what the objection to exposing this is?

matthiasbeyer commented 3 years ago

The objection is mainly as it makes interior from the implementation public. This would mean that changing the implementation of the crate is always a breaking change.

You would implement the interpolation on your config structs, so there wouldn't be any recursion involved!

polivbr commented 3 years ago

I don't have a config struct. I'm trying to make this generic and have it sit on top of the raw config. The deserialization into structs would happen against the already interpolated values.

I'm writing an example of what I'm think of against a fork of the library that exposes ValueKind. I'll link to it when it's complete so that you can see.

Exposing this would only affect users that are actually using it. Since the crate is distributed as source and is rebuilt by the consuming library or application, anyone who is currently just using the existing API won't be impacted.

matthiasbeyer commented 3 years ago

Wouldn't a proc macro (serde-with) be a solution for this problem?

polivbr commented 3 years ago

I'm not sure how that helps me. Here's a proof-of-concept that I wrote:

https://github.com/polivbr/config-rs/tree/interpolation/examples/interpolation

matthiasbeyer commented 3 years ago

I'm not sure how that helps me.

Check out this codebase. It is an extension that can be used with serde-with to expand environment variables when deserializing. The same technique can be used to interpolate values, IMO. Maybe some fiddling needs to be done to pass some context (the variables to interpolate with) to the deserialization helper, but the general idea is there.

polivbr commented 3 years ago

Except I don't have a struct that I'm deserializing into. I just want the ability to do things like config.get_string("connectionStrings.database") and get an interpolated string back.

All of this seems like an incredible amount of jumping through hoops just to avoid exposing useful functionality that hasn't changed for years. serde_json has almost identical functionality with their Value enum, and they have no qualms about exposing it.

Even if you decide to change the implementation later, the crates are versioned so the user can decide if they want to stay at the old version or port their code to the new version.

polivbr commented 3 years ago

you could also hide it behind a feature guard

matthiasbeyer commented 3 years ago

@szarykott If we expose this type, would it conflict with your ideas of async config sources? I mean, would changes be user-visible by the async-config-changes? Other issues you could think of?

polivbr commented 3 years ago

Hi, any updates on this?

mjte-riot commented 3 years ago

Hi @matthiasbeyer - to answer your previous question if this is still desired in 0.11: it is for us. We have a rust library within a C++ application, and are exposing configuration data through an FFI API. Exposing a call in this API to read config values with known types is straight forward, but it is not so easy to be able to fetch configuration data that is a table of generic/unknown structure.

The ideal solution for me would be to have Value implement ser::Serialize. This allows it to maintain the abstraction that you are hoping for. It's a fairly simple change since Value is quite compatible, and if you are not opposed I could provide a PR. I suspect this would also solve the problem for @polivbr. By implementing ser::Serialize, the FFI API can return unstructured configuration using the standard portable formats (json, etc).

If implementing Serialize is not desirable to you, then exposing ValueKind allows me to solve the problem more elegantly than trying to deduce the underlying types and structure through brute force.

matthiasbeyer commented 3 years ago

Okay, so it seems that there's use for a pub ValueKind.

I'll accept a PR for this, but I would like to see a big doc comment for the type that normal users of the crate might never have to use the type directly! :+1:

mjte-riot commented 3 years ago

Thanks @matthiasbeyer, I have a PR posted. Please let me know if the doc comment is sufficient

polivbr commented 3 years ago

Thanks @mjte-riot! I was just pulling together a PR, but you beat me to it!

matthiasbeyer commented 3 years ago

Thank you all for your contribution (not only as in code, but also as in discussion)!

I bet you want me to release another version of this crate as soon as possible, but I'd like to have the async config source and the other things from the milestone in the release. I won't add more things to the release, though. So as soon as the issues in the 0.12.0 milestone are done, I'll do a release. You can speed this up, of course, by providing review on the PRs and comments (where you see fit) on the issues.

Please go ahead and test the exposed ValueKind using the master branch of this repository and if you can extract a minimal usecase from your crates, feel free to PR me some tests/examples for that!

mjte-riot commented 3 years ago

Thanks @matthiasbeyer - I'll integrate this locally on our side and submit a PR for an example usage.