Refactor string_to_dict to return None if there is no match instead of raising ValueError
#7435
Conversation
…f raising ValueError instead of having the pattern of using try-except to handle when there is no match, we can instead check if the return value is None; we can also assert that the return value should not be None if we know that should be true
| if repo_split not in repo_splits: | ||
| repo_splits.append(split) |
There was a problem hiding this comment.
This looked like a mistake
| elif value.get("bytes") is not None or value.get("path") is not None: | ||
| # store the video bytes, and path is used to infer the video format using the file extension | ||
| return {"bytes": value.get("bytes"), "path": value.get("path")} | ||
| elif isinstance(value, dict): |
There was a problem hiding this comment.
I added this indentation because pyright isn't able to narrow the negative of the VideoReader case bc of the VideoReader is not None.
| path, bytes_ = value["path"], value["bytes"] | ||
| if isinstance(value, str): | ||
| path, bytes_ = value, None | ||
| else: | ||
| path, bytes_ = value["path"], value["bytes"] | ||
|
|
There was a problem hiding this comment.
The docstring said that value could just be the str path, but that case was not handled.
I took the liberty of creating the Example TypedDict to try to improve the typing in this class.
|
cc: @lhoestq |
|
@lhoestq any thoughts here? |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
It looks like I was unsafely asserting that |
|
Can we re-run CI on this one? |
|
Sweet! These failures are looking spurious due to connectivity issues. Can the failing run be retried? |
|
@lhoestq Sorry to double ping, but can this PR be reviewed? I think it is ready! |
Making this change, as encouraged here:
Dataset.mapto reuse cache files mapped with differentnum_proc#7434 (comment)instead of having the pattern of using
try-exceptto handle when there is no match, we can instead check if the return value isNone; we can also assert that the return value should not beNoneif we know that should be true