-
Notifications
You must be signed in to change notification settings - Fork 906
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
Use new names as type hints for deprecated classes #2788
Conversation
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.
Good catch, LGTM 👍🏽
DataSetError: type[Exception] | ||
DataSetNotFoundError: type[DatasetError] | ||
DataSetAlreadyExistsError: type[DatasetError] | ||
DataSetError: type[DatasetError] |
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.
Ugh, I get these come from the previous iteration... good catch
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.
I mean, I wrote them this way intentionally, I just didn't (couldn't?) know Mypy wouldn't like it when doing import DataSetError as DatasetError
. I feel like the type hints are technically correct (they're basically the same as for the alias), but it seems Mypy requires that the hint be the alias when doing this sort of assignment. So, I personally don't feel it was due to an oversight/rush on the previous PR, but rather just something that comes up when adding additional functionality.
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.
oh I understand, gotcha. thanks for the explanation
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.
hah, it took all night
Description
Fix to enable kedro-org/kedro-viz#1447.
Development notes
Checklist
RELEASE.md
file