python3 fixes for tracer.py iteritems, Readme #16
Conversation
There was a problem hiding this comment.
Thanks for the contribution, there's a few things that I think need changing.
(I was reading through http://python-future.org/compatible_idioms.html)
|
|
||
| @tracer.trace() | ||
| def some_view_func(request): | ||
| new_request = some_http_request | ||
| current_span = tracer.get_span(request) | ||
| text_carrier = {} | ||
| opentracing_tracer.inject(span, opentracing.Format.TEXT_MAP, text_carrier) | ||
| for k, v in text_carrier.iteritems(): | ||
| for k, v in listitems(text_carrier): |
There was a problem hiding this comment.
Shouldn't we preserve the existing behaviour of not converting to a list?
e.g.
from future.utils import iteritems
...
for k, v in iteritems(text_carrier):
| @@ -64,7 +65,7 @@ def _apply_tracing(self, request, view_func, attributes): | |||
| ''' | |||
| # strip headers for trace info | |||
| headers = {} | |||
| for k,v in request.META.iteritems(): | |||
| for k,v in listitems(request.META): | |||
There was a problem hiding this comment.
again, stick with iteritems here?
There was a problem hiding this comment.
well we can't stick with iteritems because iteritems doesn't exist in Python 3. The simplest approach I've used is just using .items() because:
- Python3 still uses iterators
- it's not a big deal to not have an iterator because it's not that big
- Only Python2 has to deal with that and no one really cares about Python2 anymore
| @@ -51,6 +52,6 @@ def client_child_span(request): | |||
| def inject_as_headers(tracer, span, request): | |||
| text_carrier = {} | |||
| tracer._tracer.inject(span.context, opentracing.Format.TEXT_MAP, text_carrier) | |||
| for k, v in text_carrier.iteritems(): | |||
| for k, v in listitmes(text_carrier): | |||
There was a problem hiding this comment.
and here.
Also - this has a typo in any case
|
The issues mentioned here are solved easily with |
|
Fixed by #25 - let me know if there's something else pending, etc ;) |
changing d.iteritems() to listitems(d) to allow python3 django apps
Running 2to3 on django_opentracing shows a few other possible fixes, mostly related to urllib2 but i don't yet have a python2 env to test so this PR is for the minimal changes required for me to get opentracing working (jaeger client).
Resolves #15