[chore] Wire up check_widget_policies correctly in dynamic containers#14143
[chore] Wire up check_widget_policies correctly in dynamic containers#14143sfc-gh-lwilby merged 3 commits intodevelopfrom
Conversation
Made-with: Cursor Co-authored-by: lawilby <[email protected]>
Made-with: Cursor Co-authored-by: lawilby <[email protected]>
sfc-gh-lwilby
added
change:chore
✅ PR preview is ready!
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Made-with: Cursor Co-authored-by: lawilby <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR resolves a deferred follow-up item from the dynamic container callback PRs by correctly wiring up check_widget_policies in st.tabs, st.expander, and st.popover. Previously, these functions called check_widget_policies with hardcoded on_change=None and enable_check_callback_rules=False, preventing the proper validation of callback usage inside forms.
Changes:
- Updated
st.tabs,st.expander, andst.popoverto follow the established pattern from chart elements (vega_charts.py, plotly_chart.py) for handling callable vs non-callableon_changeparameters - Added comprehensive test coverage to verify that callable
on_changeinsidest.formraisesStreamlitInvalidFormCallbackErrorwhileon_change="rerun"continues to work
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/streamlit/elements/layouts.py |
Updated tabs, expander, and popover functions to evaluate is_callback = callable(on_change) and pass it to enable_check_callback_rules in check_widget_policies, ensuring proper form callback validation |
lib/tests/streamlit/elements/layouts_test.py |
Added 6 regression tests (2 per container type) verifying that callable on_change inside forms raises errors while on_change="rerun" does not |
sfc-gh-bnisco
left a comment
sfc-gh-bnisco
left a comment
There was a problem hiding this comment.
The code LGTM, but it seems like the PR description is referring to TODOs and other items that don't seem to be reflected in the actual code changes.
Thanks for pointing this out, there were TODO comments but it seems they were removed in develop already and the PR description was wrong. |
Describe your changes
The partial change that landed with callable
on_changesupport usedcallable(on_change)inline in thecheck_widget_policiescall for all three dynamic containers, but without theis_callbackintermediate variable orcast. This aligns all three with the established pattern fromvega_charts.py:is_callback = callable(on_change)as a named variable for clarityon_change=cast("WidgetCallback", on_change) if is_callback else Nonefor type correctnessenable_check_callback_rules=is_callback(functionally equivalent, but consistent with the pattern)Screenshot or video (only for visual changes)
N/A
GitHub Issue Link (if applicable)
Surfaced in review of #13910.
Testing Plan
lib/tests/streamlit/elements/layouts_test.py, covering all three containers:test_callable_on_change_inside_form_raises— verifies a callableon_changeinsidest.formraisesStreamlitInvalidFormCallbackError(expander, tabs, popover)test_on_change_rerun_inside_form_does_not_raise— verifieson_change="rerun"insidest.formis still allowed (expander, tabs, popover)Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.