Simplify Matplotlib scraper #1241
Conversation
|
Interesting, the Pillow writer doesn't support |
QuLogic
force-pushed
the
simplify-scraper
branch
from
62c066d to
fe5b180
Compare
| ) | ||
| srcset_mult_facs = set() | ||
| for st in srcset: | ||
| if not isinstance(st, str) or st[-1:] not in ("", "x"): |
There was a problem hiding this comment.
Question, why the logic order re-shuffle here?
There was a problem hiding this comment.
The previous version did not check types, so this adds that, and I wanted to only have one raise.
sphinx_gallery/scrapers.py
Outdated
| extension of the output file (currently only 'png', 'jpg', 'svg', | ||
| 'gif', and 'webp' are supported). | ||
|
|
||
| This is not used internally, but intended to be used when overriding |
There was a problem hiding this comment.
| This is not used internally, but intended to be used when overriding | |
| This is not used internally, but intended for use when overriding |
Small nitpick
There was a problem hiding this comment.
That is not grammatically correct, unless you mean "for use when..."?
There was a problem hiding this comment.
Yes, sorry I've amended. I just want to make clearer that it is API available 'for' the user to use.
| @@ -80,7 +80,7 @@ def test_save_matplotlib_figures(gallery_conf, ext): | |||
| def test_save_matplotlib_figures_hidpi(gallery_conf): | |||
| """Test matplotlib hidpi figure save.""" | |||
| ext = "png" | |||
| gallery_conf["image_srcset"] = ["2x"] | |||
| gallery_conf["image_srcset"] = [2] | |||
There was a problem hiding this comment.
Sorry, I'm confused, 'image_srcset' does not have to be a string with 'x' at the end?
There was a problem hiding this comment.
It does, but now it's normalized in the configuration initialization, to avoid doing that on every scrape. But these tests appear to skip that step, so I needed to set it in its normalized form. I could add that initialization here instead (and actually maybe that should be tested as well.)
There was a problem hiding this comment.
Ahh right, that is confusing.
I could add that initialization here instead
+1
actually maybe that should be tested as well.)
+1
There was a problem hiding this comment.
Done, though this spilled over into some other tests. Not sure if that warrants a separate PR.
Instead of on every call to the scraper.
Instead of looping through all animations to match each figure, use a dictionary to map back to them.
These are for external users.
QuLogic
force-pushed
the
simplify-scraper
branch
from
fe5b180 to
992c3aa
Compare
|
Thanks @QuLogic ! |
| "Must be a list of strings with the multiplicative " | ||
| 'factor followed by an "x". e.g. ["2.0x", "1.5x"]' | ||
| ) | ||
| if st == "": |
There was a problem hiding this comment.
@QuLogic was just looking at our check config func - why do we want to allow item in gallery_conf["image_srcset"] to be an empty string?
There was a problem hiding this comment.
That preserved the previous code; I think it's always been that way since its initial PR, but as of #808 (comment), I think that was dropped from the documentation or usage, but the config handling part never did.
There was a problem hiding this comment.
Ah thank you 🙏 , I have a vague memory of empty string being a config option previously now.
Three things:
image_srcsetonce during configuration, instead of repeatedly on every scraper run. Note, I also passed the scaling numbers through a set, in order to prevent saving extra times.kwargstomatplotlib_scraper. I was confused why nothing seemed to call it withkwargs, but was able to find it in other parts of the documentation. So I mentioned that in the docstring as well.