Standardize parameter mapping APIs #4386
Conversation
|
I would like to see all the type changes be made in another PR. |
|
Fair, just to clarify that is for review purposes, to avoid merge conflicts or something else? |
|
Review purposes and to be able to look at the PR in the future if needed. |
|
Done, please review in that case 😄 |
Codecov Report
@@ Coverage Diff @@
## branch-1.0 #4386 +/- ##
==============================================
- Coverage 73.53% 73.47% -0.07%
==============================================
Files 237 238 +1
Lines 34527 34857 +330
==============================================
+ Hits 25390 25611 +221
- Misses 9137 9246 +109
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Want to make progress elsewhere (and merge this into the UI test PR) so I'll probably go ahead and merge. This is extremely difficult to review but if you do find the time/desire to do so I'll apply any fixes in a new PR. |
| @@ -616,6 +625,8 @@ class ListPanel(ListLike, Panel): | |||
| Whether to add scrollbars if the content overflows the size | |||
| of the container.""") | |||
|
|
|||
| _rename: ClassVar[Mapping[str, str | None]] = {'scroll': None} | |||
There was a problem hiding this comment.
Mapping[str, str | None] is used a lot maybe we should make it a type alias?
| @@ -30,7 +33,7 @@ class Alert(Markdown): | |||
|
|
|||
| priority: ClassVar[float | bool | None] = 0 | |||
|
|
|||
| _rename: ClassVar[Mapping[str, str | None]] = dict(Markdown._rename, alert_type=None) | |||
| _rename: ClassVar[Mapping[str, str | None]] = {'alert_type': None} | |||
There was a problem hiding this comment.
Why was Markdown._rename removed?
| Defines the PaneBase class defining the API for panes which convert | ||
| objects to a visual representation expressed as a bokeh model. | ||
| Defines base classes for Pane components which allow wrapping a Python | ||
| object transforming it into a Bokeh model that can be rendered. |
There was a problem hiding this comment.
Would probably update with:
...object and transforming...
...that can be rendered in the browser.
| @@ -1651,6 +1683,7 @@ def _get_template(self) -> Tuple[str, List[str], Mapping[str, List[Tuple[str, Li | |||
| html = html.replace('${%s}' % child_name, '') | |||
| return html, parser.nodes, p_attrs | |||
|
|
|||
| @property | |||
| def _linked_properties(self) -> List[str]: | |||
| @@ -274,7 +268,7 @@ class Number(ValueIndicator): | |||
| title_size = param.String(default='18pt', doc=""" | |||
| The size of the title given by the name.""") | |||
|
|
|||
| _rename: ClassVar[Mapping[str, str | None]] = {} | |||
| _rename: ClassVar[Mapping[str, str | None]] = {'name': 'name'} | |||
There was a problem hiding this comment.
I'm not sure I follow this rename... Is it because the parent class rename name?
Over time we accumulated a bunch of workaround for mapping parameters to properties. Specifically we had the
_renamedictionary used for mapping from parameter to property names, then we had various_init_paramsand_get_propertiesmethods used to provide parameters to initialize and update properties on the underlying bokeh model. This PR attempts to rationalize this a bit:_renameconsistently used to declare parameter mapping (i.e. avoid manual workarounds for this)_get_propertieswhich itself calls_init_paramsand then transforms those parameters with_process_param_change, ensuring that as much logic for mapping from Panel parameters to Bokeh properties is encapsulated in this method.ModelPanebaseclass for panes that very simply map to an underlying bokeh model and apply some transformations to parameters.