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

fix(parser): Improve types for parser.py #419

Merged

Conversation

carlos-alberto
Copy link
Contributor

When attempting to use the parse function we noticed we were getting
error messages like:

Value of type variable "Model" of "parse" cannot be "Type[UserModel]"

Further investigation (using the example shown at
https://awslabs.github.io/aws-lambda-powertools-python/latest/utilities/parser/#envelopes)
showed that the types were incorrect for the example. The function
signatures were expecting an instance of Model, however the example
(and code) seemeed to expect a Type[Model].

Similarly the signatures were expecting an optional instance of an
Envelope, but the code allows for either an instance or a Type.

Finally, added a TypeVar for the decorator, specifying that the
event_parser` decorator returns the same type as the handler used.

Description of changes:

Fix type signatures of parser.py

Checklist

Breaking change checklist

RFC issue #:

  • Migration process documented
  • Implement warnings (if it can live side by side)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

When attempting to use the parse function we noticed we were getting
error messages like:

    Value of type variable "Model" of "parse" cannot be "Type[UserModel]"

Further investigation (using the example shown at
https://awslabs.github.io/aws-lambda-powertools-python/latest/utilities/parser/#envelopes)
showed that the types were incorrect for the example. The function
signatures were expecting an instance of `Model`, however the example
(and code) seemeed to expect a `Type[Model]`.

Similarly the signatures were expecting an optional instance of an
Envelope, but the code allows for either an instance or a Type.

Also, added a `TypeVar` for the decorator, specifying that the
event_parser` decorator returns the same type as the handler used.

Finally updated documentation on use of parser.
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2021

Codecov Report

Merging #419 (5bd51ee) into develop (213caed) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #419   +/-   ##
========================================
  Coverage    99.89%   99.89%           
========================================
  Files          101      101           
  Lines         3978     3979    +1     
  Branches       198      198           
========================================
+ Hits          3974     3975    +1     
  Misses           1        1           
  Partials         3        3           
Impacted Files Coverage Δ
aws_lambda_powertools/utilities/parser/parser.py 100.00% <100.00%> (ø)

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 213caed...5bd51ee. Read the comment docs.

@heitorlessa
Copy link
Contributor

This is awesome @carlos-alberto!

Thanks a lot for taking time to make your first contribution.

@pcolazurdo in case you have some cycles to review.

I'm working on the new event handler docs and can look into this next. Either way, this can make into this week's release

@pcolazurdo
Copy link

As @heitorlessa said, thank you for your contribution @carlos-alberto - The code changes seems ok to me, thanks for catching these, including the mistake in the documentation.
Reflecting on this, it looks like we should add some mypy tests coverage too, to capture these in the future. Thoughts?

@heitorlessa heitorlessa added area/utilities bug Something isn't working labels May 5, 2021
@heitorlessa
Copy link
Contributor

heitorlessa commented May 5, 2021

Reflecting on this, it looks like we should add some mypy tests coverage too, to capture these in the future. Thoughts?

@pcolazurdo 100%. It's something we wanted to do, including fixing all other MyPy issues across the code base. I hadn't done that for largely 2 reasons: 1/ Bandwidth, 2/ MyPy has some false positives and need to wrestle with config and learn how to "tame" it before adding in the CI, since that would block some contributions for the unfamiliar with it.

Merging this now, thanks a lot to both of you! It'll be available in tomorrow's release.

@heitorlessa heitorlessa added this to the 1.15.0 milestone May 5, 2021
@heitorlessa heitorlessa merged commit d044463 into aws-powertools:develop May 5, 2021
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.

4 participants