Skip to content

fix: raise exception when type of DocList is object #1794

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.

Sign up for GitHub

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

Merged
JoanFM merged 2 commits into from
Sep 19, 2023
Merged

fix: raise exception when type of DocList is object #1794

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8266238
fix: raise exception when type of DocList is object
punndcoder28 Sep 16, 2023
2c41809
Merge branch 'main' into fix_bug_doclist_type
punndcoder28 Sep 19, 2023
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
13 changes: 10 additions & 3 deletions docarray/array/doc_list/doc_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

from pydantic import parse_obj_as
from typing_extensions import SupportsIndex
from typing_inspect import is_union_type
from typing_inspect import is_union_type, is_typevar

from docarray.array.any_array import AnyDocArray
from docarray.array.doc_list.io import IOMixinDocList
Expand Down Expand Up @@ -337,8 +337,15 @@ def __class_getitem__(cls, item: Union[Type[BaseDoc], TypeVar, str]):

if isinstance(item, type) and safe_issubclass(item, BaseDoc):
return AnyDocArray.__class_getitem__.__func__(cls, item) # type: ignore
else:
return super().__class_getitem__(item)
if (
Copy link
Member

@JoanFM JoanFM Sep 18, 2023

Choose a reason for hiding this comment

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

should we better have an elif?

Copy link
Contributor Author

@punndcoder28 punndcoder28 Sep 18, 2023

Choose a reason for hiding this comment

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

Since each condition has a terminating condition from the function wouldn't multiple if's be similar to elif? If there was a scenario like below an elif would definitely make it much better. In the above case, we check the instance of item and based on the condition we return a type or raise an exception. But if the code base follows if ... elif ... else blocks I would love to keep it uniform so that it is easy to maintain

computation
if condn1:
   computation = value1
elif condn2:
   computation = value2
else:
   computation = value3

return computation

isinstance(item, object)
and not is_typevar(item)
and not isinstance(item, str)
and item is not Any
):
raise TypeError('Expecting a type, got object instead')

return super().__class_getitem__(item)

def __repr__(self):
return AnyDocArray.__repr__(self) # type: ignore
Expand Down
10 changes: 10 additions & 0 deletions tests/units/array/test_array.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,3 +487,13 @@ def test_legacy_doc():
newDoc = LegacyDocument()
da = DocList[LegacyDocument]([newDoc])
da.summary()


def test_parameterize_list():
from docarray import DocList, BaseDoc

with pytest.raises(TypeError) as excinfo:
doc = DocList[BaseDoc()]
assert doc is None

assert str(excinfo.value) == 'Expecting a type, got object instead'