-
Notifications
You must be signed in to change notification settings - Fork 52
Andrew/refactor tab api #394
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
Refactors often touch a lot of parts, and this is a pretty big refactor. We reduce code deplication, improve errors and logic, and simply task management.
Some of these may have been legit errors found by pyright, some of it is just pyright not accepting certain patterns.
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.
@ayjayt Can you add docstrings to the functions in this file?
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 think we're at a decent time to start docing things yeah, now that its stable
src/py/CHANGELOG.txt
Outdated
| v1.3.0 | ||
| - Significant refactor, better organization | ||
| - `write_fig` and `_from_object` now take an additional argument: | ||
| `cancel_on_error: bool`. If False, default True, returns a list of errors. |
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.
@ayjayt Does cancel_on_error default to False or True? Is there a docstring anywhere listing the accepted arguments and their defaults?
|
@ayjayt Your changes deprecating the |
|
free threaded failing due to orjson dep, i think that's okay, its a unique use case, I made an issue: https://github.com/plotly/Kaleido/actions/runs/20118129180/job/57732318221 |
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| python_v: ['3.8', '3.9', '3.10', '3.11', '3.12', '3.13', ''] | ||
| python_v: ['3.8', '3.9', '3.10', '3.12', '3.13', '3.14', '3.14t'] |
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.
| python_v: ['3.8', '3.9', '3.10', '3.12', '3.13', '3.14', '3.14t'] | |
| python_v: ['3.8', '3.9', '3.10', '3.12', '3.13', '3.14'] |
@ayjayt Let's drop 3.14t from the CI until it runs successfully. (Or mark as expected to fail, if GHA provides a way to do that.) Keep the CI green.
emilykl
left a comment
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 good to me @ayjayt, all good to merge if you are ready 🚀
|
@ayjayt Is this ready to merge? |
Refactors internal Kaleido tab API to remove code duplication and properly split functionality into separate modules.
Mostly no user-facing changes, except as outlined below under "API changes."
API changes
kaleido.write_fig,kaleido.write_fig_sync,kaleido.write_fig_from_object,kaleido.write_fig_from_object_synccancel_on_errorfor multi-image generation:False(default): continue generating even if one image fails (previously, if an image failed generation, an error would be surfaced in the asyncio task, but generation would continue)True: if one image generation fails with error, surface error immediately to surrounding functionNone):cancel_on_error=False: returns tuple of Exceptions corresponding to failed image generations (if no failures, returns a zero-length tuple)cancel_on_error=True: returnsNone(or raises an exception)kaleido.calc_figandkaleido.calc_fig_sync:pathargument deprecated (it was previously unused, EXCEPT as one way of setting the output file type). Anyone using thepathargument to set output file type should usecalc_fig(opts=dict(format="...", ...))insteadRemove
error_logargument from all of the above functions (undocumented, probably unused)