Fix default width/height with implicit plot output #792
Conversation
Previously, default width/height value from render.plot (None) was passed to implicit plot output. With this fix, use MISSING as the default parameter value to make it clear that the default value is not set and shouldn't be passed through.
| R = TypeVar("R") | ||
|
|
||
|
|
||
| def decorator_args_passthrough( |
There was a problem hiding this comment.
This was an unused function, from an older implementation that I moved away from after @wch code review feedback.
| assert abs(rect["height"] - 200) <= tolerance | ||
|
|
||
|
|
||
| def test_decorator_passthrough_size(): |
There was a problem hiding this comment.
Moved this from e2e test to regular pytest, since it didn't actually use any playwright logic.
| assert rendered == str(ui.output_plot("foo", width=1280, height=960)) | ||
|
|
||
|
|
||
| def test_decorator_plot_default(): |
There was a problem hiding this comment.
This is the test case that reflects the issue the PR is meant to fix.
shiny/render/_render.py
Outdated
| width: Optional[float] | MISSING_TYPE = MISSING, | ||
| height: Optional[float] | MISSING_TYPE = MISSING, |
There was a problem hiding this comment.
Is None still a valid value?
If so, the intent would be clearer with float | None | MISSING_TYPE. Also, do the docs explain what None means?
| width: Optional[float] | MISSING_TYPE = MISSING, | |
| height: Optional[float] | MISSING_TYPE = MISSING, | |
| width: float | None | MISSING_TYPE = MISSING, | |
| height: float | None | MISSING_TYPE = MISSING, |
If not, take out the Optional.
| width: Optional[float] | MISSING_TYPE = MISSING, | |
| height: Optional[float] | MISSING_TYPE = MISSING, | |
| width: float | MISSING_TYPE = MISSING, | |
| height: float | MISSING_TYPE = MISSING, |
There was a problem hiding this comment.
I think the difference only matters when you're using the implicit output. None means the implicit output shouldn't have any width/height attribute, MISSING means the implicit output should use its default width/height attribute. I can't really document that until we launch express mode, maybe for now I just say None or MISSING mean the same?
* main: (56 commits) Add actions step for nightly reporting -> Testrail (#774) Remove unused entry in manifest Bump version to 0.6.0.9000 Use latest htmltools Bump version to v0.6.0; Rearrange news to have API changes last Tweaks from testing Fix default width/height with implicit plot output (#792) Update deps (#794) Remove deprecated 'name' parameter from `Outputs` (#791) api(ui): Drop `toggle_` methods. Consolidate update accordion methods. Stronger typing for `layout_sidebar(sidebar)` and `page_sidebar(sidebar)` (#788) bug(sidebar): Revert sidebar icon back to chevron (#789) For `input_action_button`, default to having whitespace around button (#758) Remove output from template app (#775) Add output_args and suspend_display decorators (#786) Update value_box; Update to bootstrap 5.3; Update htmldeps (#772) tests: add sidebar test (#787) Seaborn plots should fill their output_plot (#785) Kwargs to uvicorn run (#780) Add width and height arguments to `@render.plot` (#783) gitgnore dist/ ...
Previously, default width/height value from render.plot (None) was passed to implicit plot output. With this fix, use MISSING as the default parameter value to make it clear that the default value is not set and shouldn't be passed through.