Skip to content

Allow py310 style annotations (PEP563) when using from __future__ import annotations #184

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 2 commits into from
Jan 1, 2022

Conversation

basnijholt
Copy link
Contributor

@basnijholt basnijholt commented Oct 13, 2021

This allows one to annotate in Python 3.10 style (PEP563) when from __future__ import annotations is used.

e.g.:

from __future__ import annotations

def f(x: int | None) -> int: pass

will work

@basnijholt
Copy link
Contributor Author

@agronholm, what do you think about this?

This is relevant for all projects that use pyupgrade and future annotations, where it (for example) updates all Optional[str] to str | None.

@agronholm
Copy link
Collaborator

Sure, I will take a look later. I will need this for my own projects too.

@basnijholt basnijholt force-pushed the pep563 branch 4 times, most recently from 9e4ddf1 to 139b5b9 Compare October 13, 2021 12:02
@agronholm
Copy link
Collaborator

This PR isn't going to work as-is because Python 3.6 is still supported and that future import doesn't work there.

@basnijholt
Copy link
Contributor Author

Do you have any ideas/suggestions to make it work?

Dropping 3.6 is one solution. Many packages in the scientific Python community follow NEP 29. Even though a package might still support 3.6, I imagine no actively developed package still used 3.6 to build their docs.

@agronholm
Copy link
Collaborator

Even though a package might still support 3.6, I imagine no actively developed package still used 3.6 to build their docs.

The PyPI statistics don't support that supposition.

An extra "dummy module" for 3.7+ only could work.

@basnijholt
Copy link
Contributor Author

Good suggestion, I will try that tomorrow!

@basnijholt basnijholt force-pushed the pep563 branch 5 times, most recently from f12395d to 267ba40 Compare October 14, 2021 14:27
@basnijholt
Copy link
Contributor Author

@agronholm, I have added a dummy module that is tested only for Python ≥ 3.7.

@basnijholt basnijholt force-pushed the pep563 branch 2 times, most recently from a41ad9f to 959fa8c Compare October 14, 2021 14:38
@agronholm
Copy link
Collaborator

Maybe you should first make sure the linting checks pass locally?

@basnijholt
Copy link
Contributor Author

Done. I have also renamed pre-commit-config.sample.yaml -> .pre-commit-config.yaml to make pre-commit work.

Not sure how it would have worked before.

@agronholm
Copy link
Collaborator

Done. I have also renamed pre-commit-config.sample.yaml -> .pre-commit-config.yaml to make pre-commit work.

Not sure how it would have worked before.

The idea was that anyone wishing to use pre-commit would have a template, but would not be required to use the exact versions there.

Comment on lines 261 to 262
if sys.version_info < (3, 8):
# Only Pythoh ≥ 3.8 supports PEP563.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Umm, yes, it does. It says so in PEP 563 and I have a project that depends on that.

@basnijholt
Copy link
Contributor Author

The CI is now failing exactly like master is failing.

I can make the appropriate changes to make it pass (if the generated output is OK).

@agronholm
Copy link
Collaborator

Please do – although I would like to understand why it failed before this is merged. The same code in master passed some time before, so this is likely due to changes in Sphinx itself.

@basnijholt
Copy link
Contributor Author

basnijholt commented Oct 14, 2021

@agronholm, 5065d94 contains the changes required to make the tests pass. Unfortunately, I don't have time to figure out what changed in Sphinx to cause this change.

@basnijholt
Copy link
Contributor Author

Python 3.10 is still failing in many places, except not in the test I added test_sphinx_output_future_annotations.

@agronholm
Copy link
Collaborator

agronholm commented Oct 14, 2021

Python 3.10 is still failing in many places, except not in the test I added test_sphinx_output_future_annotations.

Those failures seem to have a singular cause – something involving Union or Optional.

@basnijholt
Copy link
Contributor Author

basnijholt commented Oct 14, 2021

I have locally tested my latest changes on Python 3.10, and now it seems like all CI jobs should pass.
In the future, we can probably use some regex to fix some of those changes, but I believe it is good enough now.

@basnijholt
Copy link
Contributor Author

Because I seem to need approval to run the CI every single time, I have set it up on my fork: https://github.com/basnijholt/sphinx-autodoc-typehints/actions/runs/1345088994.

It now all passes.

@basnijholt
Copy link
Contributor Author

Ping @agronholm 😃

@agronholm
Copy link
Collaborator

Yeah, sorry, I've been meaning to get back to this. I'll take another look tonight.

Copy link
Collaborator

@agronholm agronholm left a comment

Choose a reason for hiding this comment

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

Some change requests, some questions.

@gaborbernat
Copy link
Member

@basnijholt I've rebased on main, can you please fix the CI, thanks!

@basnijholt
Copy link
Contributor Author

I'll be AFK for more than a month. @agronholm wanted to check out the remaining failure cases I believe.

@gaborbernat gaborbernat merged commit 04f3db8 into tox-dev:main Jan 1, 2022
@basnijholt
Copy link
Contributor Author

Thanks for merging this and fixing the tests 🎉😃😄

@gaborbernat
Copy link
Member

Yeah, interestingly the future annotation has an extra zero at the end in 3.8 or later, see python/cpython@5055c27#diff-eb46a68cfb368cc943fe36ad91dba3692682ab4454221fefd6abab8757682ab0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants