Skip to content

Conversation

Yun-Kim
Copy link
Contributor

@Yun-Kim Yun-Kim commented Mar 16, 2021

Description

Following #2163, the following files were type hinted for mypy:

  • ddtrace/context.py
  • ddtrace/monkey.py
  • ddtrace/span.py
  • ddtrace/helpers.py
  • ddtrace/provider.py

Checklist

  • Added to the correct milestone.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@Yun-Kim Yun-Kim requested a review from a team as a code owner March 16, 2021 17:45
@Yun-Kim Yun-Kim added automerge changelog/no-changelog A changelog entry is not required for this PR. labels Mar 16, 2021
ddtrace/span.py Outdated
def duration(self, value):
self.duration_ns = value * 1e9
# type: (int) -> None
self.duration_ns = value * 1e9 # type: ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* 1e9 turns duration_ns into a float; perhaps it's best to either cast 1e9 to int or spell out those zeroes (the former would be more readable)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Gab, I learned something new today! 😮 I'll cast the duration_ns as an int in a separate PR but leave the type hinting as is here for now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually an error!!

datadog-agent_1  | 2021-03-16 18:44:07 UTC | TRACE | ERROR | (pkg/trace/api/api.go:379 in handleTraces) | Cannot decode v0.4 traces payload: msgp: attempted to decode type "float64" with method for "int"

@Yun-Kim Yun-Kim changed the title Enable mypy for ddtrace core, add type hinting for context, monkey, span, helpers Enable mypy for ddtrace core, add type hinting for context, monkey, span, provider, helpers Mar 16, 2021
@mergify
Copy link
Contributor

mergify bot commented Mar 16, 2021

@Yun-Kim this pull request is now in conflict 😩

@mergify mergify bot added the conflict label Mar 16, 2021
@mergify mergify bot removed the conflict label Mar 16, 2021
ddtrace/span.py Outdated

@duration.setter
def duration(self, value):
# type: (int) -> None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# type: (int) -> None
# type: (float) -> None

@mergify mergify bot merged commit 0b5f681 into DataDog:master Mar 17, 2021
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops! forgot to hit submit yesterday 🤦

"""The span duration in seconds."""
if self.duration_ns is not None:
return self.duration_ns / 1e9
# TODO: add return None if self.duration_ns is None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function returns None already in this case, so this comment can be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mypy wants an explicit return in the case self.duration_ns is None I believe, should be a quick fix in a separate PR

self.log.log(level, msg)

def trace(self, name, service=None, resource=None, span_type=None):
# type: (str, Optional[str], Optional[str], Optional[Union[str, SpanTypes]]) -> Span
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be SpanTypes still

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants