-
Couldn't load subscription status.
- Fork 75
Convert AttributeModifyingSpanExporter to Kotlin #1334
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
Convert AttributeModifyingSpanExporter to Kotlin #1334
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1334 +/- ##
==========================================
+ Coverage 64.06% 64.13% +0.07%
==========================================
Files 154 153 -1
Lines 3108 3056 -52
Branches 316 313 -3
==========================================
- Hits 1991 1960 -31
+ Misses 1028 1007 -21
Partials 89 89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| */ | ||
| class AttributeModifyingSpanExporter( | ||
| private val delegate: SpanExporter, | ||
| private val spanAttributeReplacements: (key: AttributeKey<*>, value: Any?) -> Any?, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we create an interface instead? i find it friendlier than a function def.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to use a typealias for readability
core/src/main/java/io/opentelemetry/android/export/AttributeModifyingSpanExporter.kt
Show resolved
Hide resolved
eef9b2e to
c57b7eb
Compare
| } else { | ||
| return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Prefer omitting the redundant else (and yeah, if it was kotlin it would be even cleaner to return from the if I suppose).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how this else is redundant as the lambda requires a return statement if function is null. Have I missed some syntax that would simplify this in Java?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he's talking about writing it like this:
if (function != null) {
return function.apply(value);
}
return value;Maybe it sounded like omitting the "whole else", which might be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment, but LGTM. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers!
| } else { | ||
| return value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he's talking about writing it like this:
if (function != null) {
return function.apply(value);
}
return value;Maybe it sounded like omitting the "whole else", which might be confusing.
Goal
Converts
AttributeModifyingSpanExporterto Kotlin and refactors the test to avoid unnecessary mocks.As part of this change I did alter the signature so that a single function is responsible for modifying attribute values by checking the passed in key matches.