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

refactor(examples): add from __future__ import annotations #4959

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ericbn
Copy link
Contributor

@ericbn ericbn commented Aug 13, 2024

Issue number: #4607

Summary

Changes

Add from __future__ import annotations to all examples

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 as a code owner August 13, 2024 15:49
Copy link

sonarcloud bot commented Aug 13, 2024

@ericbn
Copy link
Contributor Author

ericbn commented Aug 13, 2024

@leandrodamascena for you to have a glimpse on how all the examples changes look like and assess if it's worth it. Most changes here have been automatically done and deserve manual review. Two pending changes to this PR are:

  1. removing the from __future__ import annotations from files where no other change is required
  2. updating markdown annotations

@ericbn
Copy link
Contributor Author

ericbn commented Aug 13, 2024

❯ git show | grep '\-from typing import' | wc -l
      95

❯ git show | grep '\+from typing import .*TYPE_CHECKING' | wc -l
     292

We're arguably adding much more from typing import TYPE_CHECKING than removing or updating existing from typing import (anything).

Copy link
Contributor

No related issues found. Please ensure there is an open issue related to this change to avoid significant delays or closure.

@github-actions github-actions bot added do-not-merge need-issue PRs that are missing related issues labels Aug 14, 2024
@leandrodamascena
Copy link
Contributor

@leandrodamascena for you to have a glimpse on how all the examples changes look like and assess if it's worth it. Most changes here have been automatically done and deserve manual review. Two pending changes to this PR are:

  1. removing the from __future__ import annotations from files where no other change is required
  2. updating markdown annotations

Hi @ericbn! Sorry for the delay in responding, I had some time off today.

I'm more than happy to move forward with this PR and refactor our example to make it more pythonic. I was reading something expected for Python 3.13 and 3.14 and came across PEP 649 which seems to replace __future__ annotations in the future, but is only expected for Python 3.14.

The only thing that concerns me is the amount of examples we are changing and the possible side effect we are creating in our documentation because we will need to manually change all the highlights. Our documentation is widely used not only by those who are getting their first contact with Powertools, but also by those who are looking for some very specific examples. We know of customers who even use some examples as the base of their production code.

Changing the examples and fixing all the highlights to align with our objectives is a manual task that will take some time. Additionally, we may still make small mistakes during this process. And having a documentation that includes issues in the highlights could make the experience less engaging for our users.

We opened an issue in the Material for Mkdocs project so we could highlight using comments in the python file, but there are some challenges implementing this in other projects that Material Mkdocs uses as dependency, so we haven't been able to automate this yet.

So I would like to agree with you on the following steps:

1 - Pause this PR for now.
2 - Focus our efforts at this time on the PRs that are open for the codebase.
3 - We will release V3 in August/early September.
4 - Return to this PR and divide it into several other PRs. We can divide the efforts so that this is not extremely tiring for you.

What do you think about?

@ericbn
Copy link
Contributor Author

ericbn commented Aug 15, 2024

Oi @leandrodamascena. Totally agree with all your points! Also very good to know the challenges around highlighting.

@leandrodamascena leandrodamascena added the on-hold This item is on-hold and will be revisited in the future label Aug 15, 2024
@leandrodamascena leandrodamascena marked this pull request as draft August 15, 2024 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge enhancement need-issue PRs that are missing related issues on-hold This item is on-hold and will be revisited in the future
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants