Skip to content

[fix] Make tags type a Mapping #14

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

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Conversation

JayH5
Copy link
Contributor

@JayH5 JayH5 commented Dec 2, 2019

This reduces type errors when using the library. For example, consider a file test.py like:

from aiodogstatsd import Client

tags = {"foo": "bar"}
Client(constant_tags=tags)

...then running mypy on it:

❯ mypy test.py
test.py:4: error: Argument "constant_tags" to "Client" has incompatible type "Dict[str, str]"; expected "Optional[Dict[str, Union[float, int, str]]]"
test.py:4: note: "Dict" is invariant -- see http://mypy.readthedocs.io/en/latest/common_issues.html#variance
test.py:4: note: Consider using "Mapping" instead, which is covariant in the value type
Found 1 error in 1 file (checked 1 source file)

With this change, mypy is happy. Without this change, it's necessary to annotate variables that hold tags with the typedefs.MTags type explicitly.

@codecov-io
Copy link

codecov-io commented Dec 2, 2019

Codecov Report

Merging #14 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #14   +/-   ##
=======================================
  Coverage   96.44%   96.44%           
=======================================
  Files           7        7           
  Lines         225      225           
  Branches       16       16           
=======================================
  Hits          217      217           
  Misses          6        6           
  Partials        2        2
Impacted Files Coverage Δ
aiodogstatsd/typedefs.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28f0d07...3081122. Read the comment docs.

@Gr1N
Copy link
Owner

Gr1N commented Dec 3, 2019

Hi!

Thank you for your contribution.

Everything looks fine, but please fix a few moments:

  • update your commit message to [fix] <your message here>
  • update CHANGELOG.md

@JayH5 JayH5 force-pushed the tags-mapping-type branch from 2cd021c to 3081122 Compare December 3, 2019 12:50
@JayH5 JayH5 changed the title Make tags type a Mapping [fix] Make tags type a Mapping Dec 3, 2019
@JayH5
Copy link
Contributor Author

JayH5 commented Dec 3, 2019

@Gr1N thanks for the library

I've made the requested changes

@Gr1N Gr1N merged commit 3e0e54a into Gr1N:master Dec 3, 2019
@Gr1N
Copy link
Owner

Gr1N commented Dec 3, 2019

@JayH5 JayH5 deleted the tags-mapping-type branch December 3, 2019 13:15
@Gr1N Gr1N added the bug Something isn't working label Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants