Add pass through kopts dict for kaleido args#316
Conversation
|
|
||
| """ | ||
| async with Kaleido(n=n) as k: | ||
| if "n" not in kopts: |
There was a problem hiding this comment.
Are you wanting to keep the separate n argument for backwards compatibility? Or could it be removed?
There was a problem hiding this comment.
I would argue for removing it just to avoid confusion (since it's not obvious which way of passing n takes precedence)
There was a problem hiding this comment.
yeah i mean, i know technically we're in a release candidate but i didn't want to change the API
There was a problem hiding this comment.
I think it's fine to remove it TBH but if not, maybe add a DeprecationWarning so it can be removed in a future release?
|
|
||
| See documentation for `Kaleido.write_fig()` for the other arguments. | ||
|
|
||
| """ |
There was a problem hiding this comment.
You'll need to do a kopts = kopts or {} here, right? Otherwise the dict operations will fail if kopts is None.
There was a problem hiding this comment.
yeah
In [2]: foo(**None)
─────────────────────────────────────────
TypeError Traceback (most recent call last)
Cell In[2], line 1
────▶ 1 foo(**None)
TypeError: __main__.foo() argument after ** must be a mapping, not NoneType
There was a problem hiding this comment.
Should add kopts argument to calc_fig as well for consistency
There was a problem hiding this comment.
i want to move towards using BytesIO as a return for write_image instead of calc_fig which is sort of hackey, but added the kopts option anyway
| """ | ||
| async with Kaleido(n=1) as k: | ||
| kopts = kopts or {} | ||
| kopts["n"] = 1 |
There was a problem hiding this comment.
don't override user-provided value for n
| kopts["n"] = 1 | |
| if "n" not in kopts: | |
| kopts["n"] = 1 |
There was a problem hiding this comment.
calc_figs can't iterate, so it doesn't make sense for n>1 (its mentioned in the docs, that any value of n will be overwritten to 1)
No description provided.