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

Remove unnecessary public APIs for creating KeyValue structs #2090

Closed
utpilla opened this issue Sep 10, 2024 · 3 comments
Closed

Remove unnecessary public APIs for creating KeyValue structs #2090

utpilla opened this issue Sep 10, 2024 · 3 comments
Labels
A-metrics Area: issues related to metrics A-trace Area: issues related to tracing help wanted Good for taking. Extra help will be provided by maintainers/approvers

Comments

@utpilla
Copy link
Contributor

utpilla commented Sep 10, 2024

We have an additional set of public APIs to create KeyValue struct apart from KeyValue::new(). These APIs don't offer anything additional in terms of feature or performance. They simply offer an alternative way to create KeyValue struct by creating the Key first and then the Value instead of having them created together.

I think we could remove these APIs just to keep our public API surface smaller. I'm opening this issue to check if anyone has any opinions on this.

pub fn bool<T: Into<bool>>(self, value: T) -> KeyValue {
KeyValue {
key: self,
value: Value::Bool(value.into()),
}
}
/// Create a `KeyValue` pair for `i64` values.
pub fn i64(self, value: i64) -> KeyValue {
KeyValue {
key: self,
value: Value::I64(value),
}
}
/// Create a `KeyValue` pair for `f64` values.
pub fn f64(self, value: f64) -> KeyValue {
KeyValue {
key: self,
value: Value::F64(value),
}
}
/// Create a `KeyValue` pair for string-like values.
pub fn string(self, value: impl Into<StringValue>) -> KeyValue {
KeyValue {
key: self,
value: Value::String(value.into()),
}
}
/// Create a `KeyValue` pair for arrays.
pub fn array<T: Into<Array>>(self, value: T) -> KeyValue {
KeyValue {
key: self,
value: Value::Array(value.into()),
}
}

@utpilla utpilla added enhancement New feature or request triage:todo Needs to be traiged. labels Sep 10, 2024
@cijothomas
Copy link
Member

@TommyCpp Do you know if there is a strong reason why this should be kept?

@TommyCpp
Copy link
Contributor

TommyCpp commented Sep 10, 2024

This is mostly for ergonomics. We can remove it if we want to control API surface.

If user really wants it they can

pub trait CreateKeyValue {
   fn f64(value: f64) -> KeyValue
}

impl CreateKeyValue for Key {
      pub fn f64(self, value: f64) -> KeyValue {
        KeyValue::new(self, value)
    }
} 


// in other files
use CreateKeyValue;

let _ = Key::new("test").f64(1.0)

@cijothomas cijothomas added this to the Metrics beta release milestone Sep 11, 2024
@cijothomas cijothomas added help wanted Good for taking. Extra help will be provided by maintainers/approvers A-metrics Area: issues related to metrics A-trace Area: issues related to tracing and removed enhancement New feature or request triage:todo Needs to be traiged. labels Sep 11, 2024
@utpilla
Copy link
Contributor Author

utpilla commented Sep 18, 2024

Completed in #2091

@utpilla utpilla closed this as completed Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-metrics Area: issues related to metrics A-trace Area: issues related to tracing help wanted Good for taking. Extra help will be provided by maintainers/approvers
Projects
None yet
Development

No branches or pull requests

3 participants