Remove vega v5 wrappers #2829
Remove vega v5 wrappers #2829
Conversation
I was considering that too, and I am OK either way. I don't think we need it since this is a major release and there is an easy other way to get the same functionality via ipython.display, and we would then have this code lying around and need to delete it at a later point. But if you think it is sounder/safer to have it lying around and marked as deprecatedfor a few release, then I'm OK with that too. |
|
I changed it a bit. Now it is allowed to define Now I get a C901 error on flake8 that the |
altair/utils/save.py
Outdated
|
|
||
| if mode != "vega-lite": | ||
| if mode == "vega": | ||
| warnings.warn( |
There was a problem hiding this comment.
Same remark here about the deprecation warning
| @@ -45,7 +88,7 @@ def save( | |||
| the format to write: one of ['json', 'html', 'png', 'svg', 'pdf']. | |||
There was a problem hiding this comment.
| the format to write: one of ['json', 'html', 'png', 'svg', 'pdf']. | |
| the format to write: one of ['json', 'html', 'png', 'svg', 'pdf', 'vega', 'vega-lite']. |
Only add the vega-lite suggestion if it is also supported in the rest of save
| @@ -128,7 +148,7 @@ def save( | |||
| **kwargs, | |||
| ) | |||
| write_file_or_filename(fp, mimebundle["text/html"], mode="w") | |||
| elif format in ["png", "svg", "pdf"]: | |||
| elif format in ["png", "svg", "pdf", "vega"]: | |||
There was a problem hiding this comment.
I can't comment on the correct line but vega does not work here. It will try to access the mimebundle with key image/svg+xml on line 169.
Could also add support for "vega-lite" here?
There was a problem hiding this comment.
vega-lite cannot be supported as format. That would be json. format functions a bit like export to, or save as in other programs. If it is defined as vega it should compile the vega-lite specification into vega json.
Apparently that never has functioned.
There was a problem hiding this comment.
Of course, you're right, that would be json!
There was a problem hiding this comment.
Hm, I just noticed the following variations in the code:
https://github.com/altair-viz/altair/blob/1ed3f6f2fd3647bcf1f2e5e81dd328a0f9cf623d/altair/utils/mimebundle.py#L72-L77
It seems there is different type of json, namely, application/json and application/vnd.vegalite.v5+json (for altair 5). So I'll include vega-lite also as format, it probably would be better if it was renamed to something as vl-json.
First round of improving the requested changes. Co-authored-by: Stefan Binder <[email protected]>
|
Thanks for the continuous review @binste! I've reverted the introduction of warnings and also the enhancements to get an improved support for This PR is even with main & all tests are passing. So I'm merging. |
|
Awesome, great to see that we can already close #2817 One step closer towards a release candidate! |
https://build.opensuse.org/request/show/1113173 by user bnavigator + anag+factory - Disable altair entrypoint tests: altair 5 removed the vega wrappers * gh#vega/altair#2829 * gh#vega/ipyvega#509
Same as #2822, but renamed the brach, which closed the previous PR.
As mentioned in #2822:
Thanks for the review @joelostblom, I will update soon. Will it be better to raise deprecation warnings when vega options are called, instead of all the deletions as is proposed currently?