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

feat: support PEP604 syntax str | None #168

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: support PEP604 syntax str | None #168

wants to merge 2 commits into from

Conversation

nh13
Copy link
Member

@nh13 nh13 commented Jul 19, 2024

Fixes #89

fgpyo/util/types.py Outdated Show resolved Hide resolved
tests/fgpyo/util/test_types.py Outdated Show resolved Hide resolved
@nh13 nh13 temporarily deployed to github-actions-ci July 19, 2024 18:07 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 19, 2024 18:07 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 19, 2024 18:07 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 19, 2024 18:07 — with GitHub Actions Inactive
@nh13 nh13 temporarily deployed to github-actions-ci July 19, 2024 18:07 — with GitHub Actions Inactive
@nh13 nh13 requested a review from msto July 19, 2024 18:08
@nh13 nh13 marked this pull request as ready for review July 19, 2024 18:09
@nh13 nh13 requested a review from tfenne as a code owner July 19, 2024 18:09
Copy link

codecov bot commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.99%. Comparing base (57f13cc) to head (07a797c).

Files Patch % Lines
fgpyo/util/types.py 86.66% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   89.02%   88.99%   -0.04%     
==========================================
  Files          17       17              
  Lines        2005     2017      +12     
  Branches      437      439       +2     
==========================================
+ Hits         1785     1795      +10     
- Misses        144      145       +1     
- Partials       76       77       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tfenne
Copy link
Member

tfenne commented Jul 22, 2024

I don't think I know enough to review this well, so will rely on @msto.

@nh13
Copy link
Member Author

nh13 commented Jul 30, 2024

@msto bump

@msto msto self-assigned this Sep 12, 2024
"""Returns true if type_ is optional"""
return typing.get_origin(type_) is Union and type(None) in typing.get_args(type_)
# NB: since `_GenericAlias` is a private attribute of the `typing` module, mypy doesn't find it
TypeAnnotation: TypeAlias = Union[type, typing._GenericAlias, UnionType] # type: ignore[name-defined]
Copy link
Contributor

Choose a reason for hiding this comment

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

issue GenericAlias should be included as a valid type annotation type. Also add from types import GenericAlias above

Suggested change
TypeAnnotation: TypeAlias = Union[type, typing._GenericAlias, UnionType] # type: ignore[name-defined]
TypeAnnotation: TypeAlias = Union[type, typing._GenericAlias, UnionType,
types.GenericAlias] # type: ignore[name-defined]

Comment on lines +108 to +118
"""Check if a type is `Optional`.
An optional type may be declared using three syntaxes: `Optional[T]`, `Union[T, None]`, or `T |
None`. All of these syntaxes is supported by this function.
Args:
dtype: A type.
Returns:
True if the type is a union type with exactly two elements, one of which is `None`.
False otherwise.
Raises:
TypeError: If the input is not a valid `TypeAnnotation` type (see above).
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"""Check if a type is `Optional`.
An optional type may be declared using three syntaxes: `Optional[T]`, `Union[T, None]`, or `T |
None`. All of these syntaxes is supported by this function.
Args:
dtype: A type.
Returns:
True if the type is a union type with exactly two elements, one of which is `None`.
False otherwise.
Raises:
TypeError: If the input is not a valid `TypeAnnotation` type (see above).
"""
"""
Check if a type is `Optional`.
An optional type may be declared using three syntaxes: `Optional[T]`, `Union[T, None]`, or `T |
None`. All of these syntaxes are supported by this function.
Args:
dtype: A type.
Returns:
True if the type is a union type with exactly two elements, one of which is `None`.
False otherwise.
Raises:
TypeError: If the input is not a valid `TypeAnnotation` type.
Type annotations may be any of `type`, `types.UnionType`, `types.GenericAlias`, or `typing._GenericAlias`.
"""

# TODO When dropping support for Python 3.9, deprecate this in favor of performing instance checks
# directly on the `TypeAnnotation` union type.
# NB: since `_GenericAlias` is a private attribute of the `typing` module, mypy doesn't find it
TYPE_ANNOTATION_TYPES = (type, typing._GenericAlias, UnionType) # type: ignore[attr-defined]
Copy link
Contributor

Choose a reason for hiding this comment

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

issue as above, we should include types.GenericAlias

Suggested change
TYPE_ANNOTATION_TYPES = (type, typing._GenericAlias, UnionType) # type: ignore[attr-defined]
TYPE_ANNOTATION_TYPES = (type, typing._GenericAlias, UnionType, GenericAlias) # type: ignore[attr-defined]

Comment on lines +122 to +123
origin = typing.get_origin(dtype)
args = typing.get_args(dtype)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit I think it's more in line with our convention to import these functions instead of referencing them as module attributes

Suggested change
origin = typing.get_origin(dtype)
args = typing.get_args(dtype)
origin = get_origin(dtype)
args = get_args(dtype)

Comment on lines +22 to +28
assert not types._is_optional(str)


if sys.version_info >= (3, 10):

def test_is_optional_python_310() -> None:
assert types._is_optional(str | None)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion Save a couple lines?

Suggested change
assert not types._is_optional(str)
if sys.version_info >= (3, 10):
def test_is_optional_python_310() -> None:
assert types._is_optional(str | None)
assert not types._is_optional(str)
if sys.version_info >= (3, 10):
assert types._is_optional(str | None)

Comment on lines +21 to +22
assert types._is_optional(Optional[str])
assert not types._is_optional(str)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion Test a couple union types that don't meet our criteria

Suggested change
assert types._is_optional(Optional[str])
assert not types._is_optional(str)
assert types._is_optional(Optional[str])
assert not types._is_optional(Union[str, int])
assert not types._is_optional(Union[str, int, None])
assert not types._is_optional(str)

Copy link
Contributor

Choose a reason for hiding this comment

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

NB: for brevity (and to catch multiple failures in a single pytest invocation), you could use pytest.mark.parameterize over the tested types

@msto msto assigned nh13 and unassigned msto Oct 29, 2024
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.

Metric.read should support PEP 604 syntax for Optional types
3 participants