-
Notifications
You must be signed in to change notification settings - Fork 147
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
Conversation
nav_selector (Optional[NavSelectorType]): See [`NavSelectorType`][vizro.models.types.NavSelectorType]. | ||
Defaults to `None`. | ||
""" | ||
|
||
pages: NavPagesType = [] | ||
nav_selector: NavSelectorType = None | ||
nav_selector: Optional[NavSelectorType] = None |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be confirmed...
There was a problem hiding this comment.
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.
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 :)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #210
vizro-core/tests/unit/vizro/models/_navigation/test_navigation.py
Outdated
Show resolved
Hide resolved
nav_selector (Optional[NavSelectorType]): See [`NavSelectorType`][vizro.models.types.NavSelectorType]. | ||
Defaults to `None`. | ||
""" | ||
|
||
pages: NavPagesType = [] | ||
nav_selector: NavSelectorType = None | ||
nav_selector: Optional[NavSelectorType] = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be confirmed...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tidying it up. I left some small comments regarding the icon names. But otherwise, looks good.
vizro-core/tests/unit/vizro/models/_navigation/test_nav_item.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job with the validator. 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm! Thanks for the PR. Have a couple of small suggestions, but generally looks good
Description
className="hidden"
and replace withhidden=True
(there was one case that got back in)Screenshot
Checklist
Types of changes
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":