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
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion fgpyo/util/inspect.py
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ def list_parser(
cls: Type, type_: TypeAlias, parsers: Optional[Dict[type, Callable[[str], Any]]] = None
) -> partial:
"""
Returns a function that parses a stringified list into a `List` of the correct type.
Returns a function that parses a "stringified" list into a `List` of the correct type.

Args:
cls: the type of the class object this is being parsed for (used to get default val for
Expand Down
65 changes: 60 additions & 5 deletions fgpyo/util/types.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import collections
import inspect
import sys
import typing
from enum import Enum
from functools import partial
Expand All @@ -11,7 +12,18 @@
from typing import Union
from typing import cast

UnionType = TypeVar("UnionType", bound="Union")
from typing_extensions import TypeAlias

if sys.version_info >= (3, 10):
from types import UnionType
else:
# NB: `types.UnionType`, available since Python 3.10, is **not** a `type`, but is a class.
# We declare an empty class here to use in the instance checks below.
class UnionType:
pass


# UnionType = TypeVar("UnionType", bound="Union")
nh13 marked this conversation as resolved.
Show resolved Hide resolved
EnumType = TypeVar("EnumType", bound="Enum")
# conceptually bound to "Literal" but that's not valid in the spec
# see: https://peps.python.org/pep-0586/#illegal-parameters-for-literal-at-type-check-time
Expand Down Expand Up @@ -73,9 +85,52 @@ def is_constructible_from_str(type_: type) -> bool:
return False


def _is_optional(type_: type) -> bool:
"""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]

"""
A function parameter's type annotation may be any of the following:
1) `type`, when declaring any of the built-in Python types
2) `typing._GenericAlias`, when declaring generic collection types or union types using pre-PEP
585 and pre-PEP 604 syntax (e.g. `List[int]`, `Optional[int]`, or `Union[int, None]`)
3) `types.UnionType`, when declaring union types using PEP604 syntax (e.g. `int | None`)
4) `types.GenericAlias`, when declaring generic collection types using PEP 585 syntax (e.g.
`list[int]`)
`types.GenericAlias` is a subclass of `type`, but `typing._GenericAlias` and `types.UnionType` are
not and must be considered explicitly.
"""

# 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]



def _is_optional(dtype: TypeAnnotation) -> bool:
"""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).
"""
Comment on lines +108 to +118
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`.
"""

if not isinstance(dtype, TYPE_ANNOTATION_TYPES):
raise TypeError(f"Expected type annotation, got {type(dtype)}: {dtype}")

origin = typing.get_origin(dtype)
args = typing.get_args(dtype)
Comment on lines +122 to +123
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)


# Optional[T] has `typing.Union` as its origin, but PEP604 syntax (e.g. `int | None`) has
# `types.UnionType` as its origin.
return (
origin is not None
and (origin is Union or origin is UnionType)
and len(args) == 2
and type(None) in args
)


def _make_union_parser_worker(
Expand All @@ -84,7 +139,7 @@ def _make_union_parser_worker(
value: str,
) -> UnionType:
"""Worker function behind union parsing. Iterates through possible parsers for the union and
returns the value produced by the first parser that works. Otherwise raises an error if none
returns the value produced by the first parser that works. Otherwise, raises an error if none
work"""
# Need to do this in the case of type Optional[str], because otherwise it'll return the string
# 'None' instead of the object None
Expand Down
18 changes: 18 additions & 0 deletions tests/fgpyo/util/test_types.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,29 @@
import sys
from typing import Iterable
from typing import List
from typing import Optional
from typing import Sequence
from typing import Union

from fgpyo.util import types
from fgpyo.util.inspect import NoneType


def test_is_listlike() -> None:
assert types.is_list_like(List[str])
assert types.is_list_like(Iterable[str])
assert types.is_list_like(Sequence[str])
assert not types.is_list_like(str)


def test_is_optional() -> None:
print(type((Union[str, NoneType])))
nh13 marked this conversation as resolved.
Show resolved Hide resolved
assert types._is_optional(Union[str, NoneType])
assert types._is_optional(Optional[str])
assert not types._is_optional(str)
Comment on lines +21 to +22
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



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

def test_is_optional_python_310() -> None:
assert types._is_optional(str | None)
Comment on lines +22 to +28
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)

Loading