feat: use Cow<'static, str> instead of &'static str
#1018
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1018 +/- ##
=======================================
- Coverage 57.2% 57.2% -0.1%
=======================================
Files 143 143
Lines 17464 17471 +7
=======================================
- Hits 9999 9997 -2
- Misses 7465 7474 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
| name: &'static str, | ||
| version: Option<&'static str>, | ||
| schema_url: Option<&'static str>, | ||
| name: Cow<'static, str>, |
There was a problem hiding this comment.
What was the reason for not using impl Into<Cow<'static, str>> again?
There was a problem hiding this comment.
I can use impl Into here, but not for the Option types unfortunately, as you cannot convert &str into Option<Cow<...>>.
So the best i can do is just the first parameter for name. Would you like me to make this change?
There was a problem hiding this comment.
I think to be consistent with trace that would be good :)
| pub fn meter(name: &'static str) -> Meter { | ||
| meter_provider().versioned_meter(name, None, None) | ||
| pub fn meter(name: impl Into<Cow<'static, str>>) -> Meter { | ||
| meter_provider().versioned_meter(name.into(), None, None) |
There was a problem hiding this comment.
Nit: Can we use meter_provider().meter(name.into()) to avoid the extra Nones?
| @@ -349,13 +349,13 @@ impl trace::TracerProvider for GlobalTracerProvider { | |||
| /// Create a versioned tracer using the global provider. | |||
| fn versioned_tracer( | |||
| &self, | |||
| name: impl Into<Cow<'static, str>>, | |||
| _version: Option<&'static str>, | ||
| _schema_url: Option<&'static str>, | ||
| _name: Cow<'static, str>, | ||
| _version: Option<Cow<'static, str>>, |
There was a problem hiding this comment.
Curious as to why we are removing the & for version and schema_url? Are we allowing accepting of both String and &str for these fields?
There was a problem hiding this comment.
Yes previously it only allowed for static strings, but here it allows for Cow<'static, str>, which means it can be an owned string if the version is not static (eg might come from user input, io, etc).
| _name: impl Into<Cow<'static, str>>, | ||
| _version: Option<&'static str>, | ||
| _schema_url: Option<&'static str>, | ||
| _name: Cow<'static, str>, |
There was a problem hiding this comment.
What would be the implications of using Cow<'static, str> over impl Into<Cow<'static, str>> from a user standpoint?
Closes #1015
Uses
Cow<'static, str>instead of&'static strfor some functions.I initially tried
impl Into<Cow<'static, str>>for the types, but it seemed to complicate things when used with theOptionfor version and schema_url.