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

Simplify cattrs._compat.is_typeddict #384

Merged
merged 9 commits into from
Jun 14, 2023

Conversation

AlexWaygood
Copy link
Contributor

@AlexWaygood AlexWaygood commented Jun 13, 2023

Following up from python/typing_extensions#230 (comment)! Not sure why the test for generic typeddicts on 3.11 is failing.

This:

  • Is simpler
  • Is more robust (it doesn't involve importing private implementation details from typing/typing_extensions, which are liable to be changed without warning)
  • Doesn't add any additional dependency on typing_extensions except on Python <3.10

@@ -20,15 +20,18 @@
from attr import fields as attrs_fields
from attr import resolve_types

__all__ = ["ExtensionsTypedDict", "is_typeddict", "TypedDict"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like __all__ was deleted from this file in #382, but I'm not entirely sure why. I've added it back here, as otherwise ruff complains (reasonably) that is_typeddict is imported but unused. The alternative would be to do from typing_extensions import is_typeddict as is_typeddict, but I find that syntax really ugly personally :p

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops, my bad. You might wanna add ExceptionGroup too which uses the x as x syntax currently.

@AlexWaygood AlexWaygood reopened this Jun 13, 2023
@AlexWaygood
Copy link
Contributor Author

Hmm, the test suite is succeeding on PyPy but the "upload to codecov" bit is failing, and then that leads to all other jobs being cancelled

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #384 (97774e2) into main (05175ef) will decrease coverage by 0.01%.
The diff coverage is 72.72%.

❗ Current head 97774e2 differs from pull request most recent head 019bc63. Consider uploading reports for the commit 019bc63 to get more accurate results

❗ 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     #384      +/-   ##
==========================================
- Coverage   95.75%   95.75%   -0.01%     
==========================================
  Files          26       26              
  Lines        2122     2121       -1     
==========================================
- Hits         2032     2031       -1     
  Misses         90       90              
Impacted Files Coverage Δ
src/cattrs/_compat.py 95.45% <72.72%> (-0.48%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@Tinche
Copy link
Member

Tinche commented Jun 13, 2023

Wow Codecov is really being garbage today, sorry about that heh.

@Tinche
Copy link
Member

Tinche commented Jun 14, 2023

Here's a reduced version of the failing test case:

from typing import TypedDict, Generic, TypeVar
from cattrs._compat import is_typeddict

T = TypeVar("T")

class A(TypedDict, Generic[T], total=True):
    a: T

print(is_typeddict(A[int]))  # False, should be true

@AlexWaygood
Copy link
Contributor Author

Here's a reduced version of the failing test case:

Ahh, thank you! Should be fixed now -- all tests are now passing for me locally :)

@AlexWaygood
Copy link
Contributor Author

Also, I should have spotted that behaviour difference between cattrs's version of is_typeddict and the typing(_extensions) one just by looking at the code 🤦

AlexWaygood added a commit to AlexWaygood/cattrs that referenced this pull request Jun 14, 2023
This will make it easier to see in python-attrs#384 which tests (if any) are actually failing on which Python versions -- currently the codecov failures are just leading to all jobs being cancelled in CI
Tinche pushed a commit that referenced this pull request Jun 14, 2023
This will make it easier to see in #384 which tests (if any) are actually failing on which Python versions -- currently the codecov failures are just leading to all jobs being cancelled in CI
@Tinche Tinche added this to the 23.2 milestone Jun 14, 2023
@Tinche
Copy link
Member

Tinche commented Jun 14, 2023

LGTM, <3 can you just add a line to the changelog? Doesn't have to be very specific, but might help someone down the line if there's a behavior change we're not testing for.

@AlexWaygood
Copy link
Contributor Author

LGTM, <3 can you just add a line to the changelog? Doesn't have to be very specific, but might help someone down the line if there's a behavior change we're not testing for.

Done! Lmk if that's not quite what you were looking for :)

@Tinche
Copy link
Member

Tinche commented Jun 14, 2023

Great, thanks!

@Tinche Tinche merged commit aec154c into python-attrs:main Jun 14, 2023
@AlexWaygood AlexWaygood deleted the is-typeddict branch June 14, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants