Skip to content

refactor(openapi): add from __future__ import annotations #4990

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 7 commits into from
Aug 16, 2024

Conversation

ericbn
Copy link
Contributor

@ericbn ericbn commented Aug 15, 2024

Issue number: #4991

Summary

Changes

Add from __future__ import annotations to openapi package

User experience

Discussed in #4607

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

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

Acknowledgment

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

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.

and update code according to ruff rules TCH, UP006, UP007, UP037 and
FA100.
@ericbn ericbn requested a review from a team August 15, 2024 16:43
@pull-request-size pull-request-size bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 15, 2024
@ericbn
Copy link
Contributor Author

ericbn commented Aug 15, 2024

@leandrodamascena, looks like Pydantic does not work with the new type annotations out-of-the-box with from __future__ import annotations, which actually makes sense. I'm getting this error e.g. with Python 3.8:

TypeError: You have a type annotation 'str | None' which makes use of newer typing features than are supported in your version of Python. To handle this error, you should either remove the use of new syntax or install the eval_type_backport package.

    TypeError: You have a type annotation 'str | None' which makes use
    of newer typing features than are supported in your version of Python.
    To handle this error, you should either remove the use of new syntax
    or install the `eval_type_backport` package.
@leandrodamascena
Copy link
Contributor

@leandrodamascena, looks like Pydantic does not work with the new type annotations out-of-the-box with from __future__ import annotations, which actually makes sense. I'm getting this error e.g. with Python 3.8:

I think it works, but not in version 3.8/3.9. I'm running tests with Python 3.10 and 3.11 and it's working perfectly.

I don't think we have much choice here other than not to change the openapi utility. I wouldn't want to install anything new and/or refactor and risk breaking something.

I don't see Pydantic being able to fix this, as it seems more like a problem with older versions of Python. We can close this PR and keep the issue open as Tech Debt so that it can be fixed one day.

What do you think?

@ericbn
Copy link
Contributor Author

ericbn commented Aug 15, 2024

The code in this PR will fail in Python 3.8/3.9. I'm going to fix it.

@ericbn
Copy link
Contributor Author

ericbn commented Aug 15, 2024

Added a new commit to fix Python 3.8/3.9

@leandrodamascena
Copy link
Contributor

Added a new commit to fix Python 3.8/3.9

I saw that you reverted some files. Let's wait for CI to complete.
The error in SonarCloud is expected and we don't need to care about this.

@boring-cyborg boring-cyborg bot added the tests label Aug 15, 2024
Copy link

@leandrodamascena
Copy link
Contributor

leandrodamascena commented Aug 15, 2024

Hi @ericbn! I see that the CI is green now and I've done an initial review and everything looks good. Tomorrow morning I'll do one last review before approving and merging.

I can't thank you enough for all the effort you've put into these PRs, it's an incredible amount of amazing work. ❤️

Copy link
Contributor

@leandrodamascena leandrodamascena left a comment

Choose a reason for hiding this comment

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

APPROVED!

@leandrodamascena leandrodamascena merged commit 689072f into aws-powertools:v3 Aug 16, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement event_handlers size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tech debt: add from __future__ import annotations in the Event Handler OpenAPI feature
2 participants