Enable mypy for ddtrace core, add type hinting for tracer and filters#2163
Conversation
| """ | ||
|
|
||
| _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set)) | ||
| _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set)) # type: Dict[str, Set] |
There was a problem hiding this comment.
I think you can do this instead:
| _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set)) # type: Dict[str, Set] | |
| _hooks = attr.ib(init=False, factory=lambda: collections.defaultdict(set), type=Dict[str, Set]) |
And it'd actually be wrong as Dict and defaultdict are different. There's a type for defaultdict:
https://docs.python.org/3/library/typing.html#typing.DefaultDict
There was a problem hiding this comment.
Thanks for the catch! 😄
|
|
||
|
|
||
| class TraceFilter(six.with_metaclass(abc.ABCMeta)): | ||
| class TraceFilter(six.with_metaclass(abc.ABCMeta)): # type: ignore[misc] |
There was a problem hiding this comment.
What was the type error here?
There was a problem hiding this comment.
Seems like all the ABCs have type errors 🤔
There was a problem hiding this comment.
Here's the mypy error: error: Unsupported dynamic base class "six.with_metaclass" [misc] This might be an issue with the ABCMeta rather than with_metaclass
|
There are a few functions in compat we didn't add type hints to, should we just do all the functions in that file, seems easy/low hanging fruit? |
brettlangdon
left a comment
There was a problem hiding this comment.
mostly nits at this point, looking good
|
|
||
|
|
||
| def get_connection_response(conn): | ||
| # type: (...) -> Response |
There was a problem hiding this comment.
| # type: (...) -> Response | |
| # type: (httplib.HTTPConnection) -> Response |
This should be the correct typing here
There was a problem hiding this comment.
Since httplib.HTTPConnection is from six.moves.httplib, which itself causes mypy issues relating to importing attributes, it's not trivial to add this specific connection typing into the typedef. Thoughts on leaving the arg type not hinted and instead clearly stating the HTTPConnection type in the docstrings @Kyle-Verhoog @brettlangdon ?
| span_type=None, # type: Optional[str] | ||
| ): | ||
| def wrap(self, name=None, service=None, resource=None, span_type=None): | ||
| # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable |
There was a problem hiding this comment.
The more detailed we can get with Callable the better.
e.g.
| # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable | |
| # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[[...], typing.Any], Callable[[...], typing.Any] |
(I think that is right, since we return the wrap_decorator which takes a function with any arguments and any return type, and returns a function which takes any arguments with any return type.... ?)
| from typing import AnyStr | ||
| from typing import Text | ||
|
|
||
| # from ddtrace.internal.writer import Response |
There was a problem hiding this comment.
| # from ddtrace.internal.writer import Response |
|
@Yun-Kim this pull request is now in conflict 😩 |
brettlangdon
left a comment
There was a problem hiding this comment.
small nits, otherwise lgtm
| def wrap(self, name=None, service=None, resource=None, span_type=None): | ||
| # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[..., Any]], Callable[..., Any]] # noqa |
There was a problem hiding this comment.
"noqa" because the line is too long? are we better following this pattern we use as well?
| def wrap(self, name=None, service=None, resource=None, span_type=None): | |
| # type: (Optional[str], Optional[str], Optional[str], Optional[str]) -> Callable[[Callable[..., Any]], Callable[..., Any]] # noqa | |
| def wrap( | |
| self, | |
| name=None, # type: Optional[str] | |
| service=None, # type: Optional[str] | |
| resource=None, # type: Optional[str] | |
| span_type=None, # type: Optional[str] | |
| ): | |
| # type: (...) -> Callable[[Callable[..., Any]], Callable[..., Any]] |
There was a problem hiding this comment.
Completely didn't occur to me I can use both type hinting styles haha, fixed
Codecov Report
@@ Coverage Diff @@
## master #2163 +/- ##
=======================================
Coverage 91.67% 91.67%
=======================================
Files 567 567
Lines 40472 40488 +16
=======================================
+ Hits 37101 37117 +16
Misses 3371 3371
Continue to review full report at Codecov.
|
Description
This enables mypy checking for ddtrace core, with the trace, filter, compat, files being type hinted.
Since not all of ddtrace core has been type hinted, this ignores some mypy errors (will be fixed in upcoming PRs)
Checklist