Move contextFromString out of JaegerSpanContext into TextMapCodec #517
Conversation
Signed-off-by: Isaac Hier <ihier@uber.com>
|
As usual, Travis fails due to crossdock timeout. |
| @@ -54,6 +57,25 @@ private TextMapCodec(Builder builder) { | |||
| this.baggagePrefix = builder.baggagePrefix; | |||
| } | |||
|
|
|||
| public static JaegerSpanContext contextFromString(String value) | |||
There was a problem hiding this comment.
If it's used only in tests, can't we make this private and test the inject operation instead?
There was a problem hiding this comment.
Isn't the point of the internal package to imply anything here may change without warning? Public makes this easily accessible in a number of test modules without compromising our external API.
There was a problem hiding this comment.
This has been blocking the other PR, so I would rather not cause major refactoring of the tests. But going for minimum visibility is always a good goal. In this case, wouldn't all the tests work just as well if this was package level viz?
There was a problem hiding this comment.
I tried it and sadly no. propagation is its own package.
There was a problem hiding this comment.
I can probably fix the other tests. Don't merge yet.
There was a problem hiding this comment.
Oops too late. Put another one if a simple fix.
Codecov Report
@@ Coverage Diff @@
## master #517 +/- ##
===========================================
- Coverage 88.31% 88.2% -0.11%
Complexity 499 499
===========================================
Files 65 65
Lines 1849 1849
Branches 239 239
===========================================
- Hits 1633 1631 -2
- Misses 139 141 +2
Partials 77 77
Continue to review full report at Codecov.
|
Fixes #518.