-
Notifications
You must be signed in to change notification settings - Fork 476
Added mypy type hinting to hooks, worker, encoding, payload, pin, sampler #2188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ddtrace/sampler.py
Outdated
|
|
||
| def __init__(self, sample_rate=1): | ||
| def __init__( | ||
| self, sample_rate=1 # type: float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| self, sample_rate=1 # type: float | |
| self, sample_rate=1.0 # type: float |
|
@Yun-Kim this pull request is now in conflict 😩 |
ddtrace/internal/_encoding.pyi
Outdated
| content_type: str | ||
| def _decode(self, data: Union[str, bytes]) -> Any: ... | ||
| def encode_trace(self, trace: List[Any]) -> bytes: ... | ||
| def join_encoded(self, objs: List[bytes]) -> Union[str, bytes]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Union[str, bytes]? It should just be bytes I'd think 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I assumed it should have the same return type as the input type for _decode, but makes sense I'll change that!
ddtrace/sampler.py
Outdated
| """ | ||
|
|
||
| def __init__(self, sample_rate=1): | ||
| def __init__(self, sample_rate=1.0): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this change separately? There are further usages below that use ints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds fair to me, I'll address this in a separate PR then 👍
Description
Following #2180, the following files were type hinted for mypy:
Checklist