-
Notifications
You must be signed in to change notification settings - Fork 212
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
Should config::get panic on configuration error or return Result ? #8
Comments
Careful - compile-time cfgs are required to be purely additive so you cannot do: #[cfg(not(feature = "panic"))]
pub fn get<'a>(key: &str) -> Option<&'a Value>
#[cfg(feature = "panic")]
pub fn get<'a>(key: &str) -> &'a Value This is not purely additive because it removes the function with the first signature. Imagine if my crate A uses config without the panic feature, so it calls config::get().unwrap(). Then I add a dependency on a crate B which uses config with the panic feature. When Cargo resolves the features in my dependency graph, it will build my crate A against config with the panic feature enabled and fail because unwrap() is not a method on &'a Value. |
You're completely right. Let's change this issue then into a .. what does the community want as the method signature? pub fn get<'a>(key: &str) -> Result<&'a Value, ConfigError> Or pub fn get<'a>(key: &str) -> &'a Value |
Personally I would want: pub fn get<T: Deserialize>(key: &str) -> Result<T, Error> and for my use cases, I never see myself ever using any of the other possible signatures.
|
@dtolnay Perhaps you can settle an internal dilemma. Do people care about allocation when using a config library? Eg.. pub fn get_str(key: &str) -> Result<String, Error> Or pub fn get_str<'a>(key: &str) -> Result<&'a str, Error> The main reason for the Unsafe #9 and other weirdness is I have it stuck in my head that this matters and we should try to return a reference if possible. |
I am sure nobody cares about an allocation. If you need to use a config value inside a hot loop, you load it outside the loop and refer to it from inside the loop. Even with the current reference-based API you would never want to call get inside of a hot loop because it will be slower than calling it once up front. |
@dtolnay That settles that. Thank you for all your help so far. |
I would also prefer get returning a |
I was actually expecting the signature to just be Result, but that a panic would occur if the feature flag was set (because some applications wouldn't have a sensible way of handling an absent config). Though I guess just unwrapping would make sense! Sorry, I didn't think it completely through when I mentioned the idea. |
Yeah. Going to close this as we seem to all agree. |
@nugend definitely possible and something I'd probably use.
The text was updated successfully, but these errors were encountered: