Skip to content

Commit

Permalink
[check] dont allow str or bytes for sequence_param (#25262)
Browse files Browse the repository at this point in the history
match the behavior pydantic went with here
pydantic/pydantic#5595 which I think is
preferable

## How I Tested These Changes

updated tests
  • Loading branch information
alangenfeld authored Oct 15, 2024
1 parent 27b841b commit ce0288b
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 6 deletions.
7 changes: 6 additions & 1 deletion python_modules/dagster/dagster/_check/functions.py
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,12 @@ def sequence_param(
) -> Sequence[T]:
ttype = type(obj)
# isinstance check against abc is costly, so try to handle common cases with cheapest check possible
if not (ttype is list or ttype is tuple or isinstance(obj, collections.abc.Sequence)):
if not (
ttype is list
or ttype is tuple
# even though str/bytes are technically sequences, its likely not desired so error
or (isinstance(obj, collections.abc.Sequence) and ttype not in (str, bytes))
):
raise _param_type_mismatch_exception(
obj, (collections.abc.Sequence,), param_name, additional_message
)
Expand Down
4 changes: 2 additions & 2 deletions python_modules/dagster/dagster/_core/instance/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2324,10 +2324,10 @@ def delete_dynamic_partition(self, partitions_def_name: str, partition_key: str)
Args:
partitions_def_name (str): The name of the `DynamicPartitionsDefinition`.
partition_key (Sequence[str]): Partition key to delete.
partition_key (str): Partition key to delete.
"""
check.str_param(partitions_def_name, "partitions_def_name")
check.sequence_param(partition_key, "partition_key", of_type=str)
check.str_param(partition_key, "partition_key")
self._event_storage.delete_dynamic_partition(partitions_def_name, partition_key)

@public
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1115,8 +1115,6 @@ def test_sequence_param():
assert check.sequence_param(tuple(), "sequence_param") == tuple()

assert check.sequence_param(["foo"], "sequence_param", of_type=str) == ["foo"]
assert check.sequence_param("foo", "sequence_param") == "foo"
assert check.sequence_param("foo", "sequence_param", of_type=str) == "foo"

with pytest.raises(ParameterCheckError):
check.sequence_param(None, "sequence_param")
Expand All @@ -1127,6 +1125,12 @@ def test_sequence_param():
with pytest.raises(CheckError):
check.sequence_param(["foo"], "sequence_param", of_type=int)

with pytest.raises(CheckError):
check.sequence_param("foo", "sequence_param")

with pytest.raises(CheckError):
check.sequence_param("foo", "sequence_param", of_type=str)


def test_opt_sequence_param():
assert check.opt_sequence_param([], "sequence_param") == []
Expand Down Expand Up @@ -1616,7 +1620,7 @@ class SubGen(Gen[str]): ...
(Bar, [Bar()], [Foo()]),
(Optional[Bar], [Bar()], [Foo()]),
(List[str], [["a", "b"]], [[1, 2]]),
(Sequence[str], [["a", "b"]], [[1, 2]]),
(Sequence[str], [["a", "b"]], [[1, 2], "just_a_string"]),
(Iterable[str], [["a", "b"]], [[1, 2]]),
(Set[str], [{"a", "b"}], [{1, 2}]),
(AbstractSet[str], [{"a", "b"}], [{1, 2}]),
Expand Down

0 comments on commit ce0288b

Please sign in to comment.