Add typing to tracer.py#328
Add typing to tracer.py#328yurishkuro merged 6 commits intojaegertracing:masterfrom kasium:typing_tracer
Conversation
Codecov Report
@@ Coverage Diff @@
## master #328 +/- ##
==========================================
+ Coverage 95.51% 95.52% +0.01%
==========================================
Files 25 25
Lines 2030 2035 +5
Branches 274 274
==========================================
+ Hits 1939 1944 +5
Misses 57 57
Partials 34 34
Continue to review full report at Codecov.
|
|
Please note, that for most types I checked the tests and the docu, but I'm no expert in your code. Feel free to check and to provide better/correct types. |
yurishkuro
left a comment
There was a problem hiding this comment.
few nits, otherwise looks great
This change was done by using pyannotate and running the test_tracer tests. When in doubt, the type is now Any. Signed-off-by: Kai Mueller <15907922+kasium@users.noreply.github.com>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Kai Mueller <15907922+kasium@users.noreply.github.com>
Signed-off-by: Kai Mueller <15907922+kasium@users.noreply.github.com>
jaeger_client/tracer.py
Outdated
| tags.get(ext_tags.SPAN_KIND) == ext_tags.SPAN_KIND_RPC_SERVER | ||
| rpc_server = cast( | ||
| bool, tags and tags.get(ext_tags.SPAN_KIND) == ext_tags.SPAN_KIND_RPC_SERVER | ||
| ) |
There was a problem hiding this comment.
when I run make lint it works fine without this change. How are you testing typing?
There was a problem hiding this comment.
Did you also remove "bool(rpc_server)" from the other line. I see then
jaeger_client/tracer.py:223: error: Argument "join" to "_emit_span_metrics" of "Tracer" has incompatible type "Union[Dict[Any, Any], None, Any]"; expected "Optional[bool]"
There was a problem hiding this comment.
$ make lint
flake8 jaeger_client crossdock tests
mypy jaeger_client
Success: no issues found in 36 source files
./scripts/check-license.sh
$ gd | cat
diff --git a/jaeger_client/tracer.py b/jaeger_client/tracer.py
index e4e8e00..e553f6e 100644
--- a/jaeger_client/tracer.py
+++ b/jaeger_client/tracer.py
@@ -22,6 +22,8 @@ import random
import sys
import time
import opentracing
+from typing import Optional
+
from opentracing import Format, UnsupportedFormatException
from opentracing.ext import tags as ext_tags
from opentracing.scope_managers import ThreadLocalScopeManager
@@ -282,7 +284,7 @@ class Tracer(opentracing.Tracer):
self.sampler.close()
return self.reporter.close()
- def _emit_span_metrics(self, span, join=False):
+ def _emit_span_metrics(self, span: Span, join: Optional[bool] = False):
if span.is_sampled():
self.metrics.spans_started_sampled(1)
else:
There was a problem hiding this comment.
I did pip install mypy
mypy-0.910
mypy_extensions-0.4.3
There was a problem hiding this comment.
So, of course if you remove the type annotation of the argument, mypy will not complain 😄
There was a problem hiding this comment.
I do have the type declared in the function. I also tried with my original suggestion, still successful:
rpc_server: bool = tags and \
tags.get(ext_tags.SPAN_KIND) == ext_tags.SPAN_KIND_RPC_SERVER
There was a problem hiding this comment.
This one should fail, because a cast is needed. Not sure, why it's successful on your side. On mine it fails as expected.
Maybe a weird mypy cache issue or something else in your setup. Might also be related to the python version which you running.
However, making a cast is fine, so I see no risk here
There was a problem hiding this comment.
Yes, but the cast is ugly code. What errors are you getting with rpc_server: bool = ?
I'm running with Python 3.8.2
There was a problem hiding this comment.
jaeger_client/tracer.py:182: error: Incompatible types in assignment (expression has type "Union[Dict[Any, Any], None, Any]", variable has type "bool")
Even a clean doesn't help
There was a problem hiding this comment.
Ok, found a way to write it easier
Signed-off-by: Kai Mueller <15907922+kasium@users.noreply.github.com>
This change was done by using pyannotate and running the test_tracer tests.
When in doubt, the type is now Any.
Part of #319
Signed-off-by: Kai Mueller 15907922+kasium@users.noreply.github.com