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.

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

RFC: Including the Typing returns in Metrics #642

Closed
mkamioner opened this issue Aug 22, 2021 · 4 comments
Closed

RFC: Including the Typing returns in Metrics #642

mkamioner opened this issue Aug 22, 2021 · 4 comments
Assignees
Labels
good first issue Good for newcomers help wanted Could use a second pair of eyes/hands p1 RFC

Comments

@mkamioner
Copy link

Key information

  • RFC PR: (leave this empty)
  • Related issue(s), if known:
  • Area: Metrics
  • Meet tenets: (Yes/no)
  • Approved by: ''
  • Reviewed by: ''

Summary

The Metrics library does not include the return types of the functions, causing MyPy to complain.

Motivation

Adding typing to the return of the signature of the function will help prevent errors during development. Also, Mypy will stop complaining:

Proposal

Go through the metrics/metrics.py file and add the return type to all functions

User Experience

This would require no code change and be backwards compatible. Users would get a better result from mypy and type hinting tools to help makde development easier

Before

/file.py: note: In member "record_metrics" of class "ConnectionCountRecorderDriver":
/file.py:26:9: error: Call to untyped function "clear_metrics" in typed context  [no-untyped-call]
Found 1 errors in 1 file (checked 1 source file)

After

Success: no issues found in 1 source file

Drawbacks

Honestly, none that I can think of except for someone's time - happy to take care of that :-)

Rationale and alternatives

  • What other designs have been considered? Why not them? Not sure what else has been considered, but since we use python typing in other places in the same file, it makes sense to be consistent.
  • What is the impact of not doing this? Better developer experience 💪

Unresolved questions

Optional, stash area for topics that need further development e.g. TBD

@mkamioner mkamioner added RFC triage Pending triage from maintainers labels Aug 22, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Aug 22, 2021

Thanks for opening your first issue here! We'll come back to you as soon as we can.

@heitorlessa
Copy link
Contributor

Hey @mkamioner, we absolutely agree and if you could help us make MyPy compliant we'd appreciate it immensely!

We've fixed roughly 600 of the initial MyPy findings[1].... there's still 35+ from my last count to be fixed (Metrics included).

[1] aws-powertools/powertools-lambda#3

@heitorlessa heitorlessa added help wanted Could use a second pair of eyes/hands good first issue Good for newcomers and removed triage Pending triage from maintainers labels Aug 22, 2021
@heitorlessa heitorlessa added this to the 1.21.0 milestone Sep 28, 2021
@heitorlessa heitorlessa self-assigned this Oct 1, 2021
@heitorlessa heitorlessa removed this from the 1.21.0 milestone Oct 1, 2021
@heitorlessa
Copy link
Contributor

hey @mkamioner - While I couldn't reproduce that particular error with Metrics (a snippet would be great), I've gone ahead and added return types -> None, really to all of them, and fixed other pending areas.

In the overall project we've now got 13 mypy issues to fix but that should in theory address your problem in the next release.

Thanks!

@heitorlessa heitorlessa added the pending-release Fix or implementation already in dev waiting to be released label Oct 1, 2021
@heitorlessa
Copy link
Contributor

This is now available as part of 1.21.0, do let us know otherwise (and if you do, please share a snippet and mypy config to help us reproduce)

https://github.com/awslabs/aws-lambda-powertools-python/releases/tag/v1.21.0

@heitorlessa heitorlessa removed the pending-release Fix or implementation already in dev waiting to be released label Oct 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Could use a second pair of eyes/hands p1 RFC
Projects
None yet
Development

No branches or pull requests

2 participants