-
Notifications
You must be signed in to change notification settings - Fork 12
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 some minor typing issues #592
Conversation
encord/dataset.py
Outdated
@@ -260,7 +260,7 @@ def create_image_group( | |||
|
|||
def create_dicom_series( | |||
self, | |||
file_paths: List[str], | |||
file_paths: Union[List[str], List[Path], List[Union[Path, str]]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/List/Iterable/g
if we're going for the most lenient definition for users convenience, like in the other method. Otherwise just Iterable[Path]
instead of the whole union in both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterable makes sense and in the spirit of keeping it as flexible as possible. Can probably be Iterable[Union[Path,str]] (unless mypy throws a fit?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Guys have a good point about accepting Iterable or Sequence rather than List type, but otherwise +1!
dcac0f9
to
e71accc
Compare
Unit test report ((Pydantic 2.x)189 tests 189 ✅ 6s ⏱️ Results for commit e71accc. |
Unit test report (Pydantic 1.x)189 tests 189 ✅ 5s ⏱️ Results for commit e71accc. |
SDK integration test report271 tests 271 ✅ 23m 58s ⏱️ Results for commit e71accc. |
Introduction and Explanation
A little tidy-up so that we can declare it
py.typed
and have thesdk_integration_tests
be properly typed, too.Known issues
There are some unfixable-ish issues anyway, but let's fix what's easy to fix.