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

Minor update to Navigation docs #190

merged 19 commits into from
Dec 13, 2023

Conversation

huong-li-nguyen
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen commented Dec 4, 2023

Description

  • Fix typing in docstrings
  • Remove remaining className="hidden" and replace with hidden=True (there was one case that got back in)
  • Consolidate formatting for default values in docstrings
  • Add validator for provided icon names (transforms to lower-case and replaces spaces with underscores)

Screenshot

Checklist

  • I have not referenced individuals, products or companies in any commits, directly or indirectly
  • I have not added data or restricted code in any commits, directly or indirectly
  • I have updated the docstring of any public function/class/model changed
  • I have added tests to cover my changes (if applicable)

Types of changes

  • Docs/refactoring (non-breaking change which improves codebase)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.

@huong-li-nguyen huong-li-nguyen self-assigned this Dec 4, 2023
@huong-li-nguyen huong-li-nguyen added the Docs 🗒️ Issue for markdown and API documentation label Dec 4, 2023
@huong-li-nguyen huong-li-nguyen marked this pull request as ready for review December 4, 2023 13:42
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

vizro-core/docs/pages/user_guides/navigation.md Outdated Show resolved Hide resolved
vizro-core/docs/pages/user_guides/navigation.md Outdated Show resolved Hide resolved
vizro-core/src/vizro/models/_navigation/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
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

@nadijagraca nadijagraca left a 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.

Copy link
Contributor

@nadijagraca nadijagraca left a 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. 🚀

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a 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

@huong-li-nguyen huong-li-nguyen merged commit 864bafe into main Dec 13, 2023
25 checks passed
@huong-li-nguyen huong-li-nguyen deleted the tidy/update_nav_docs branch December 13, 2023 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs 🗒️ Issue for markdown and API documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants