Skip to content

Fix/pep 563 class init annotations parsing#107

Merged
adriangb merged 7 commits into
adriangb:mainfrom
maxzhenzhera:fix/pep-563-class-init-annotations-parsing
Sep 8, 2023
Merged

Fix/pep 563 class init annotations parsing#107
adriangb merged 7 commits into
adriangb:mainfrom
maxzhenzhera:fix/pep-563-class-init-annotations-parsing

Conversation

@maxzhenzhera

@maxzhenzhera maxzhenzhera commented Aug 31, 2023

Copy link
Copy Markdown
Contributor

Closes #105

@codecov-commenter

codecov-commenter commented Aug 31, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #107 (4704902) into main (133d696) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the GitHub App Integration for your organization. Read more.

@@            Coverage Diff             @@
##             main     #107      +/-   ##
==========================================
- Coverage   96.64%   96.62%   -0.03%     
==========================================
  Files          67       67              
  Lines        2266     2280      +14     
  Branches      328      331       +3     
==========================================
+ Hits         2190     2203      +13     
- Misses         66       67       +1     
  Partials       10       10
Files Changed Coverage Δ
di/_utils/inspect.py 100.00% <100.00%> (ø)
tests/test_wiring_pep_563.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

adriangb
adriangb reviewed Sep 1, 2023
View reviewed changes
Comment thread di/_utils/inspect.py Outdated
Comment on lines +70 to +73
"""
For no apparent reason, `Annotated[Optional[T]]` comes back as `Optional[Annotated[Optional[T]]]`
so remove the outer `Optional` if this is the case.
"""

@adriangb adriangb Sep 1, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This seems really strange. Can you make a MRE? If so we should submit it as a bug to CPython.

@maxzhenzhera maxzhenzhera Sep 2, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I only refactored this code (moved the block that handles this logic to a function):

di/di/_utils/inspect.py

Lines 59 to 65 in 133d696

# for no apparent reason, Annotated[Optional[T]] comes back as Optional[Annotated[Optional[T]]]
# so remove the outer Optional if this is the case
for param_name, hint in hints.items():
args = get_args(hint)
if get_origin(hint) is Union and get_origin(next(iter(args))) is Annotated:
hints[param_name] = next(iter(args))
return hints

This code is yours: 6fce6ca

@adriangb adriangb Sep 2, 2023

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Ha okay, I’ll look into it!

@maxzhenzhera maxzhenzhera Sep 8, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue that the code is fixing: python/cpython#90353
I put it in the function docstring.

@adriangb adriangb merged commit 4d94386 into adriangb:main Sep 8, 2023
@adriangb

adriangb commented Sep 8, 2023

Copy link
Copy Markdown
Owner

Thank you so much for the contribution!

@maxzhenzhera maxzhenzhera deleted the fix/pep-563-class-init-annotations-parsing branch September 30, 2023 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adriangb adriangb adriangb approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: pep 563 (class __init__ annotations parsing)

3 participants

@maxzhenzhera @codecov-commenter @adriangb