-
-
Notifications
You must be signed in to change notification settings - Fork 835
fix: type issues with Chart save methods. #3934
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
Conversation
c846e2e to
3e68cf1
Compare
|
@mattijn could you please approve the workflow for the latest commit? The original commit failed due to incompatibility with python 3.9 so I reverted that change and rebased. |
|
Approved the workflow! Looks good! And great that you work on this PR🙌. I've one question: do we need the change regarding this line: Before values: Any = UndefinedAfter values: dict[str, Any] | list | InlineDataset | None = NoneThere have been long discussions in the past on the difference between |
|
Thanks for approving, the PR passes now. Let me look more into |
|
Neither ty nor basedpyright complained about the (original) use of Undefined so I reverted that change. Might be worth adding a unit test to illustrate the difference. |
|
Nice! Can this PR be moved out of draft status? |
|
Done. PR is live and ready to review. |
|
Thanks @alec-bike! Looks great |
This reverts commit 846f091.
* Fix unknown dict type issues. * Ignore some type errors in save. * Revert typing as None over Undefined.
* Fix unknown dict type issues. * Ignore some type errors in save. * Revert typing as None over Undefined.
Chart.save()methods trigger type-check warnings with ty. This PR adds some type annotations to avoid these warnings.Resolves #3870.
To test run ty:
uv run task testreveals no change in test results compared to main.Notes:
replacing Union with | requires min python version 3.10