Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor update to Navigation docs #190

Merged
merged 19 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add Optional to NavSelectorType
  • Loading branch information
huong-li-nguyen committed Dec 4, 2023
commit fb78e851edfaf14eb228e2ab8efd18febfe4852a
14 changes: 3 additions & 11 deletions vizro-core/schemas/0.1.7.dev0.json
Original file line number Diff line number Diff line change
Expand Up @@ -954,7 +954,7 @@
},
"NavLink": {
"title": "NavLink",
"description": "Icon to be used in Navigation Panel of Dashboard.\n\nArgs:\n ages (Optional[NavPagesType]): See [`NavPagesType`][vizro.models.types.NavPagesType].\n Defaults to `[]`.\n label (str): Text description of the icon for use in tooltip.\n icon (str): Icon name from [Google Material icons library](https://fonts.google.com/icons).\n Defaults to `\"\"`.",
"description": "Icon to be used in Navigation Bar of Dashboard.\n\nArgs:\n pages (NavPagesType): See [`NavPagesType`][vizro.models.types.NavPagesType]. Defaults to `[]`.\n label (str): Text description of the icon for use in tooltip.\n icon (str): Icon name from [Google Material icons library](https://fonts.google.com/icons). Defaults to `\"\"`.",
"type": "object",
"properties": {
"id": {
Expand Down Expand Up @@ -1041,7 +1041,7 @@
},
"Navigation": {
"title": "Navigation",
"description": "Navigation in [`Dashboard`][vizro.models.Dashboard] to structure [`Pages`][vizro.models.Page].\n\nArgs:\n pages (Optional[NavPagesType]): See [`NavPagesType`][vizro.models.types.NavPagesType].\n Defaults to [].\n nav_selector (Optional[NavSelectorType]): See [`NavSelectorType`][vizro.models.types.NavSelectorType].\n Defaults to `None`.",
"description": "Navigation in [`Dashboard`][vizro.models.Dashboard] to structure [`Pages`][vizro.models.Page].\n\nArgs:\n pages (NavPagesType): See [`NavPagesType`][vizro.models.types.NavPagesType]. Defaults to `[]`.\n nav_selector (Optional[NavSelectorType]): See [`NavSelectorType`][vizro.models.types.NavSelectorType].\n Defaults to `None`.",
"type": "object",
"properties": {
"id": {
Expand Down Expand Up @@ -1073,15 +1073,7 @@
},
"nav_selector": {
"title": "Nav Selector",
"description": "Component for rendering navigation.",
"discriminator": {
"propertyName": "type",
"mapping": {
"accordion": "#/definitions/Accordion",
"nav_bar": "#/definitions/NavBar"
}
},
"oneOf": [
"anyOf": [
{
"$ref": "#/definitions/Accordion"
},
Expand Down
10 changes: 7 additions & 3 deletions vizro-core/src/vizro/models/_navigation/navigation.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from __future__ import annotations

from typing import Optional, cast

from dash import html
from pydantic import validator

Expand All @@ -15,11 +17,12 @@ class Navigation(VizroBaseModel):

Args:
pages (NavPagesType): See [`NavPagesType`][vizro.models.types.NavPagesType]. Defaults to `[]`.
nav_selector (NavSelectorType): See [`NavSelectorType`][vizro.models.types.NavSelectorType]. Defaults to `None`.
nav_selector (Optional[NavSelectorType]): See [`NavSelectorType`][vizro.models.types.NavSelectorType].
Defaults to `None`.
"""

pages: NavPagesType = []
nav_selector: NavSelectorType = None
nav_selector: Optional[NavSelectorType] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonymilne - here I wasn't sure whether the Optional was removed intentionally?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be confirmed...

Copy link
Contributor Author

@huong-li-nguyen huong-li-nguyen Dec 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reading through this section of the migration guide for pydantic V2 and this forum post I don't have a strong preference, both should work fine.

To sum up the relevant parts:

Runtime implications
In practice this means that for the following model all fields behave identically in the sense that neither is required during initialization because all of them will receive None as their value, when no other value is provided, and the type of each of them is set to be Optional[str].


from typing import Optional, Union
from pydantic import BaseModel, Field

class Model(BaseModel):
    a: str | None
    b: Union[str, None]
    c: Optional[str]
    d: Optional[str] = None
    e: Optional[str] = Field(default=None)
    f: Optional[str] = Field(default_factory=lambda: None)
    g: str = None

It seems that the g case, the one we are talking about here is under the hood converted to Optional[str] = None:

Note that even g is implicitly handled in a way that sets its type to be Optional[str], even though we annotate it with just str. You can verify this by doing the following:

for field in Model.__fields__.values():
    print(repr(field))

Migration to V2
What will change in pydantic V2 is the way how Optional[X] are handled if no defaults are provided as far as I understand e.g. Optional[str] and Optional[str] = None were both considered as non-required and a default of None was automatically set in V1.

This is not the case anymore with V2. Optional[str] without a default will be considered as a required field, while Optional[str] = None is not required, can be None, and is None by default. That being said you are right that str = None will result in the same, so that field being considered as non-required, can be None and is None by default. So functionally both are the same.

Screenshot 2023-12-06 at 11 07 31

I have a slight preference for Optional[XXX] = None if we want to be more explicit, but as you already mentioned this sometimes leads to these mypy issues where we have to solve it via cast, making it a bit difficult to understand for new developers. So it’s questionable whether this improves it. So either solutions are actually fine for me :)

Copy link
Contributor

@maxschulz-COL maxschulz-COL Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting conversation. Optional[XXX] = None seems to be a little more what python developer are used to I guess, but if it causes other problems, happy to change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed w/ @antonymilne : Changes will be reverted here and handled properly in separate PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #210


# validators
_validate_pages = validator("pages", allow_reuse=True)(_validate_pages)
Expand All @@ -35,7 +38,8 @@ def pre_build(self):

@_log_call
def build(self, *, active_page_id=None) -> _NavBuildType:
nav_selector = self.nav_selector.build(active_page_id=active_page_id)
nav_selector: NavSelectorType = cast(NavSelectorType, self.nav_selector).build(active_page_id=active_page_id)
huong-li-nguyen marked this conversation as resolved.
Show resolved Hide resolved

if "nav_bar_outer" not in nav_selector:
# e.g. nav_selector is Accordion and nav_selector.build returns single html.Div with id="nav_panel_outer".
# This will make it match the case e.g. nav_selector is NavBar and nav_selector.build returns html.Div
Expand Down
Loading