fix(api): apply_style fails fast on a non-Axes first positional#320
fix(api): apply_style fails fast on a non-Axes first positional#320dchaudhari7177 wants to merge 4 commits into
Conversation
apply_style(ax, style=None) accepted any first positional, so passing
the preset name — apply_style("SCITEX_STYLE") — sailed past the boundary
and crashed deep inside with an opaque AttributeError ('str' object has
no attribute 'set_facecolor'), which downstream try/except blocks
silently swallowed. Raise a TypeError at the boundary naming the bad
type and the correct call shape, so the wrong call fails loud with a
traceback pointing at the call site.
Fixes scitex-ai#160 (the apply_style half; the load_style auto-propagation in
scitex-ai#159 is out of scope here).
Adds regression tests for both the reject and the valid-Axes paths.
|
Please sign the SciTeX CLA before your contribution can be merged. I have read and agree to the SciTeX CLA. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
ywatanabe1989
left a comment
There was a problem hiding this comment.
Great catch, and a faithful implementation of exactly what #160 proposed — the guard, the message, and both tests are all right. We ran your tests against current main: 3/3 pass.
One case slipped through, and it is really on us: the issue's own snippet had the same blind spot, so you implemented precisely what we asked. figrecipe's own fr.subplots() returns a RecordingAxes composition wrapper (src/figrecipe/_wrappers/_axes.py — mixins delegating through __getattr__, not an Axes subclass). On current main, fr.apply_style(ax) with that wrapper works via delegation; with the strict isinstance guard it now raises TypeError. Our internal style path already unwraps first (src/figrecipe/_api/_subplots.py:219), and the same one-liner fixes it here — see the suggested change below.
Could you also add a third test pinning the wrapper case, so nobody re-breaks it?
def test_apply_style_accepts_figrecipe_wrapped_axes():
import figrecipe as fr
fig, ax = fr.subplots()
try:
result = fr.apply_style(ax)
assert isinstance(result, float)
finally:
import matplotlib.pyplot as plt
plt.close("all")Everything else is merge-ready. (Separately, whenever convenient: the CLA bot's one-comment signature is the only other thing needed before merge — opening and review needed none.)
| # Fail loud at the boundary: a non-Axes first positional (commonly a preset | ||
| # name passed as `apply_style("SCITEX_STYLE")`) otherwise crashes deep | ||
| # inside with an opaque `'str' object has no attribute ...` (#160). | ||
| if not isinstance(ax, matplotlib.axes.Axes): |
There was a problem hiding this comment.
| # Fail loud at the boundary: a non-Axes first positional (commonly a preset | |
| # name passed as `apply_style("SCITEX_STYLE")`) otherwise crashes deep | |
| # inside with an opaque `'str' object has no attribute ...` (#160). | |
| if not isinstance(ax, matplotlib.axes.Axes): | |
| # figrecipe's own subplots() returns a RecordingAxes wrapper (composition, | |
| # not an Axes subclass) — unwrap it first so `fig, ax = fr.subplots(); | |
| # fr.apply_style(ax)` keeps working, matching the unwrap in _subplots.py. | |
| ax = getattr(ax, "_ax", ax) | |
| # Fail loud at the boundary: a non-Axes first positional (commonly a preset | |
| # name passed as `apply_style("SCITEX_STYLE")`) otherwise crashes deep | |
| # inside with an opaque `'str' object has no attribute ...` (#160). | |
| if not isinstance(ax, matplotlib.axes.Axes): |
fr.subplots() returns a RecordingAxes composition wrapper (delegates via __getattr__, not an Axes subclass), so the strict isinstance guard rejected it. Unwrap to ._ax first, mirroring the style path in _subplots.py. Adds a test pinning the wrapper case.
|
Good catch — fixed. On the CLA: that's a legal sign-off my collaborator needs to make on their own account, so I've flagged it to them to complete. |
ywatanabe1989
left a comment
There was a problem hiding this comment.
Perfect — the unwrap is exactly the idiom from _subplots.py, placed before the guard so wrappers pass and a stray string still fails loud, and the new wrapper test pins it. Approving.
For the CLA: our bot checks the commit author account, and your commits are authored as dipakchaudhari12717 — so that's the account that needs to post the one-line signing comment (here or on any of your three PRs; one signature covers all SciTeX repos). Once it's in, we'll merge all three. Thanks for the quick turnaround — three solid PRs on day one is a great start.
|
@dchaudhari7177 — full note on #318 (covers all three). In short: the CI red here was on our side — I updated your branch to current main to fix it — and the only remaining step is the CLA sign-off comment from |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Symptom
apply_style(ax, style=None)accepted any first positional, so a preset name passed where the Axes belongs sailed past the API boundary and crashed deep inside with an opaqueAttributeError. Downstreamtry/except Exceptionwrappers silently swallowed it — the exact silent-fallback the issue calls out.Fix
A
TypeErrorat the boundary, per the issue's proposed shape:I pointed the hint at the existing
style=argument rather thanload_style('SCITEX'), since that auto-propagation is #159's scope and isn't in this repo yet — keeping this PR to just theapply_stylehalf the issue explicitly separates out. Easy to reword to referenceload_stylewhen #159 lands.Tests
test__style_manager.pygains two regression tests: the string first-positional raisesTypeErrormatchingmust be a matplotlib Axes, and a real Axes still returns a positive float trace width.pytest tests/figrecipe/_api/→ 32 passed.Fixes #160