-
Notifications
You must be signed in to change notification settings - Fork 324
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
Make public API setters fluent #294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
============================================
+ Coverage 71.43% 71.65% +0.21%
+ Complexity 1202 1197 -5
============================================
Files 133 132 -1
Lines 4631 4568 -63
Branches 474 471 -3
============================================
- Hits 3308 3273 -35
+ Misses 1108 1080 -28
Partials 215 215
Continue to review full report at Codecov.
|
This is pretty awesome. To support method chaining in 0.7 I was creating extension methods in Kotlin. |
Ideally, we should also remove the |
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.
Add test that ensures chaining calls are working as expected
return null; | ||
} | ||
|
||
void doSetName(String name) { |
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.
maybe protected
would be a more accurate accessor to these methods?
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 think so, as package private is more restrictive. IntelliJ suggests removing the protected
qualifier.
Any update on this? |
It needs some changes to be compatible with the old API which has void return types. After the changes it can be merged :) |
thanks for the ping btw |
I have confirmed that this also works when using version |
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.
Great. Just go over all comments and make sure you submit it with the new package names, e.g. co.elastic.apm.agent.plugin
instead of co.elastic.apm.plugin
, and watch out not to add the agent
package to co.elastic.apm.api
|
||
@Override | ||
public ElementMatcher<? super TypeDescription> getTypeMatcher() { | ||
return named("co.elastic.apm.api.SpanImpl").and(not(hasSuperType(named("co.elastic.apm.api.AbstractSpanImpl")))); |
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.
Nice
depends on #293