Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add mypy typing to datadog_checks_base for db utils #7867

Closed
wants to merge 1 commit into from
Closed

Add mypy typing to datadog_checks_base for db utils #7867

wants to merge 1 commit into from
+23 −5

Conversation

justiniso
Copy link
Contributor

What does this PR do?

As a follow-up to #7837, add the mypy typing to the added functions.

Motivation

This was requested as part of the PR, but I decided to make it a fast follow-on to reduce risk.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@justiniso justiniso requested review from a team as code owners October 27, 2020 18:49
@codecov
Copy link

codecov bot commented Oct 27, 2020 •
edited
Loading

Codecov Report

Merging #7867 into master will increase coverage by 0.50%.
The diff coverage is n/a.

Impacted Files Coverage Δ
system_swap/tests/test_integration.py
datadog_checks_base/tests/test_aws.py
datadog_checks_base/tests/test_wmisampler.py
...se/datadog_checks/base/utils/metadata/constants.py
datadog_checks_base/tests/test_metadata.py
system_swap/tests/common.py
...tool/datadog_checks/cassandra_nodetool/__init__.py
..._checks_base/datadog_checks/base/utils/tailfile.py
...atadog_checks/base/utils/prometheus/metrics_pb2.py
gitlab/tests/conftest.py
... and 149 more

hithwen
hithwen previously approved these changes Oct 28, 2020
View reviewed changes
@hithwen
Copy link
Contributor

hithwen commented Oct 28, 2020

Style is failing. You can fix it with ddev test -fs datadog_checks_base

florimondmanca
florimondmanca reviewed Oct 28, 2020
View reviewed changes
datadog_checks_base/datadog_checks/base/utils/db/sql.py
"""
Given an already obfuscated & normalized SQL query, generate its 64-bit hex signature.
"""
if not normalized_query:
return None
# Note: please be cautious when changing this function as some features rely on this
# hash matching the APM resource hash generated on our backend.
return format(mmh3.hash64(normalized_query, signed=False)[0], 'x')
return format(mmh3.hash64(normalized_query, signed=False)[0], str('x'))
Copy link
Contributor

Choose a reason for hiding this comment

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

str('x')

This looks weird, what is the motivation for this syntax?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I'm using from __future__ import unicode_literals, the 'x' is a unicode string (py2) being passed to format, which only accepts ascii str as an argument.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay! So just to be sure, this is unrelated to typing, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, no it is due to typing. The error is from mypy, but does not produce a code execption in py2.

datadog_checks_base/datadog_checks/base/utils/db/sql.py Outdated Show resolved Hide resolved
datadog_checks_base/datadog_checks/base/utils/db/statement_metrics.py Outdated Show resolved Hide resolved
@justiniso justiniso force-pushed the justiniso/mypy-typing branch from e1d23fc to 44b281a Compare October 28, 2020 15:04
florimondmanca
florimondmanca previously approved these changes Oct 28, 2020
View reviewed changes
@justiniso justiniso force-pushed the justiniso/mypy-typing branch 2 times, most recently from 2096184 to ed858ac Compare October 30, 2020 13:58
@stale
Copy link

stale bot commented Dec 25, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. Note that this will not be automatically closed, but the notification will remind us to investigate why there's been inactivity.

@stale stale bot added the stale label Dec 25, 2020
@justiniso
Add type declarations to datadog_checks_base
559dbe7 
Typo

Add db files to mypy

Fix imports

Fix typing errors

Typing fixes
@justiniso justiniso force-pushed the justiniso/mypy-typing branch from ed858ac to 559dbe7 Compare May 5, 2021 23:19
@stale stale bot removed the stale label May 5, 2021
@justiniso justiniso marked this pull request as draft May 5, 2021 23:35
@justiniso justiniso closed this Jun 14, 2021
@dd-devflow dd-devflow bot deleted the justiniso/mypy-typing branch February 7, 2024 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florimondmanca florimondmanca florimondmanca left review comments

@hithwen hithwen hithwen left review comments

Assignees
No one assigned
Labels
dev/tooling integration/datadog_checks_base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justiniso @hithwen @florimondmanca