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

env: add a 'translate_key' field to ease dealing with kebab-case #380

Closed
wants to merge 1 commit into from

Conversation

da-x
Copy link
Contributor

@da-x da-x commented Sep 28, 2022

This allows usage of kebab-case attribute in serde, allowing to mapping unambiguously into a config value given a multiple character separator.

let environment = Environment::default()
    .prefix("PREFIX")
    .translate_key(TranslationType::Kebab)
    .separator("__");

This allows usage of `kebab-case` attribute in serde, allowing
to mapping unambiguously into a config value given a multiple
character separator.

    let environment = Environment::default()
        .prefix("PREFIX")
        .translate_key(TranslationType::Kebab)
        .separator("__");
Copy link
Member

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

I really like that you added tests as well!

Please have a look at my annotations. I will approve CI right now though, so we can catch things early.

Comment on lines +8 to +11
/// An environment source collects a dictionary of environment variables values into a hierarchical
/// config Value type. We have to be aware how the config tree is created from the environment
/// dictionary, therefore we are mindful about prefixes for the environment keys, level separators,
/// encoding form (kebab, snake case) etc.
Copy link
Member

Choose a reason for hiding this comment

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

Could you please split this to a seperate commit?

@@ -77,6 +86,11 @@ pub struct Environment {
source: Option<Map<String, String>>,
}

#[derive(Clone, Debug)]
pub enum TranslationType {
Kebab, // Translate '_' to '-'
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add more here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can go even further, depending on convert_case crate and pass a convert_case::Case value here instead (eliminating TranslationType). If doing this, should we add a feature flag with optional dependency?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good, IMO. Yes, a feature flag would be nice!

Comment on lines +224 to +230
if let Some(translate_key) = translate_key {
match translate_key {
TranslationType::Kebab => {
key = key.replace("_", "-");
},
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some(translate_key) = translate_key {
match translate_key {
TranslationType::Kebab => {
key = key.replace("_", "-");
},
}
}
if let Some(TranslationType::Kebab) = translate_key {
key = key.replace("_", "-");
}

But if we add more types then this is not possible of course. 😆

@da-x
Copy link
Contributor Author

da-x commented Sep 29, 2022

@matthiasbeyer, all requested done in another PR to fix branch name mixup. Here: #381

@da-x da-x closed this Sep 29, 2022
@da-x da-x deleted the daloni/traces-enum-strings branch September 29, 2022 18:08
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