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

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

Merged
merged 2 commits into from
Sep 19, 2023

Conversation

punndcoder28
Copy link
Contributor

  • Fixes being able to pass an object for DocList without causing code to raise expection as mentioned in Bug with DocList[] #1782

Test Plan - Running the below code raises exception

from docarray import DocList, BaseDoc

doc = DocList[BaseDoc()]

Exception raise

Traceback (most recent call last):
  File "test.py", line 3, in <module>
    doc = DocList[BaseDoc()]
  File "docarray/docarray/array/doc_list/doc_list.py", line 340, in __class_getitem__
    raise TypeError(f'Expecting a type, got {item} of type {type(item)}')
TypeError: Expecting a type, got BaseDoc(id='9ab01e333a55195e5bb3f97604b97f10') of type <class 'docarray.base_doc.doc.BaseDoc'>

else:
return super().__class_getitem__(item)

if isinstance(item, object) and not isinstance(item, TypeVar):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a check isinstance(item, TypeVar) since item of TypeVar was also being passed when __class_getitem__ was called

@punndcoder28 punndcoder28 marked this pull request as ready for review September 16, 2023 18:01
@punndcoder28
Copy link
Contributor Author

@samsja @JoanFM I am unsure on how to handle the case doc = DocList[MyDoc][MyDoc] because the item parameter which gets passed to __class_getitem__ of both AnyDocArray and DocList is of type BaseDoc

@codecov
Copy link

codecov bot commented Sep 16, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.56% 🎉

Comparison is base (8f32866) 84.24% compared to head (2c41809) 84.81%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1794      +/-   ##
==========================================
+ Coverage   84.24%   84.81%   +0.56%     
==========================================
  Files         135      135              
  Lines        8899     8901       +2     
==========================================
+ Hits         7497     7549      +52     
+ Misses       1402     1352      -50     
Flag Coverage Δ
docarray 84.81% <100.00%> (+0.56%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
docarray/array/doc_list/doc_list.py 86.99% <100.00%> (+0.21%) ⬆️

... and 8 files with indirect coverage changes

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

@JoanFM
Copy link
Member

JoanFM commented Sep 17, 2023

Hey @punndcoder28, please make sure to add relevant tests for this

@JoanFM JoanFM linked an issue Sep 18, 2023 that may be closed by this pull request
@@ -337,8 +337,14 @@ 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

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

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

@punndcoder28
Copy link
Contributor Author

Will fix the failing tests related to new changes

@punndcoder28
Copy link
Contributor Author

@JoanFM Can we pass string literal as types for a class? Like so

    def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:
        return a

This is being done here

def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:

@JoanFM
Copy link
Member

JoanFM commented Sep 18, 2023

@JoanFM Can we pass string literal as types for a class? Like so

    def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:
        return a

This is being done here

def g(a: DocList['BaseDoc']) -> DocList['BaseDoc']:

u can in the type hints

Signed-off-by: punndcoder28 <puneethk.2899@gmail.com>
@JoanFM JoanFM merged commit 2f3b85e into docarray:main Sep 19, 2023
36 of 38 checks passed
@JoanFM JoanFM mentioned this pull request Oct 2, 2023
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.

Bug with DocList[]
2 participants