Add builder patter and deprecate some APIs #424
Conversation
ghost
assigned 
|
After this we could release |
Codecov Report
@@ Coverage Diff @@
## master #424 +/- ##
============================================
- Coverage 87.21% 87.07% -0.15%
+ Complexity 512 510 -2
============================================
Files 66 66
Lines 1955 1980 +25
Branches 258 258
============================================
+ Hits 1705 1724 +19
- Misses 160 166 +6
Partials 90 90
Continue to review full report at Codecov.
|
|
|
||
| /** | ||
| * @param denyBaggageOnInitializationFailure determines the startup failure mode of | ||
| * RemoteBaggageRestrictionManager. If denyBaggageOnInitializationFailure is true, |
There was a problem hiding this comment.
minor: would it not be better to move the bulk of the text above, to provide a description, and leave the @param line brief?
There was a problem hiding this comment.
I am fine with as it is. The text is copied from the constructor as it was. If you have the text add it here and I will update it.
There was a problem hiding this comment.
Good point - although I think because now the parameters are separated into their own "with..." methods, each method now has the opportunity to clearly describe what it represents. One possible way to change would be move all text from "If denyBaggageOn...." to the main description block, e.g.
/**
* If denyBaggageOnInitializationFailure is true,
* {@link RemoteBaggageRestrictionManager} will not allow any baggage to be written until
* baggage restrictions have been retrieved from agent. If denyBaggageOnInitializationFailure is
* false, {@link RemoteBaggageRestrictionManager} will allow any baggage to be written until
* baggage restrictions have been retrieved from agent.
*
* @param denyBaggageOnInitializationFailure determines the startup failure mode of RemoteBaggageRestrictionManager
*/
| /** | ||
| * @param serviceName restrictions for this service are kept track of | ||
| */ | ||
| public Builder(String serviceName) { |
There was a problem hiding this comment.
Is this now the general convention being used - as the serviceName is essentially mandatory it is a constructor parameter, but all other info is optional so provided via with... methods?
There was a problem hiding this comment.
Look at other builders requiring serviceName - Tracer.Builder, RemoteControllerSampler.Builder . Configuration. All require service name as mandatory parameter.
There was a problem hiding this comment.
thanks, I have also changed the other one
| } | ||
|
|
||
| /** | ||
| * @param initialDelayMs delay before first fetch of restrictions. It is only exposed for testing |
There was a problem hiding this comment.
minor: as above, might be better to put the explanation above as part of a description block, and just have some brief text on the param.
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
pavolloffay
force-pushed
the
deprecate-apis-baggage-manager
branch
from
11456d0
to
e5360a0
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Related to #395
Continuation of #414 (review)
Signed-off-by: Pavol Loffay ploffay@redhat.com