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.

Sign up for GitHub

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

Jump to bottom

expose NoUpdate #2800

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

expose NoUpdate #2800

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a89eb3
expose NoUpdate
2Ryan09 Mar 16, 2024
3c3a274
import NoUpdate as NoUpdateType
2Ryan09 Mar 17, 2024
8d8043c
NoUpdate -> NoUpdateType
2Ryan09 Mar 21, 2024
d40af98
missed two
2Ryan09 Mar 21, 2024
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
1 change: 1 addition & 0 deletions dash/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
from .version import __version__ # noqa: F401,E402
from ._callback_context import callback_context # noqa: F401,E402
from ._callback import callback, clientside_callback # noqa: F401,E402
from ._callback import NoUpdate as NoUpdateType # noqa: F401,E402
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be better do rename it inside _callback.py, otherwise there might be situations where the same thing shows up as either name. I wouldn't consider that a breaking change, this hasn't ever been part of the exposed API.

Copy link
Member

@ndrezn ndrezn Mar 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@KoolADE85 brought up this point as well.

Compare:

type(no_update) # <class 'dash._callback.NoUpdate'>

whereas with None, it's

type(None)      # <class 'NoneType'>

So we should expect (I'm pretty sure):

type(no_update) # <class 'dash.NoUpdateType'>

Which requires renaming the NoUpdate class.

We'll want this type to be discoverable & renaming rather than aliasing enables that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather it not be postfixed.

Copy link
Member

Choose a reason for hiding this comment

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

@T4rk1n how come?

Copy link
Contributor

Choose a reason for hiding this comment

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

What we have here is not a type but a class. Since everything is a class in Python it feel meaningless to have a postfix of Type which it is not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My main goal here is just to ensure users always use the instance rather than the class in their callbacks. If we were to expose both dash.no_update and dash.NoUpdate, then someone looking at dir(dash) could easily choose the wrong one. But if we have dash.NoUpdateType instead it would be clear not to use that one.

To my mind type vs class is a minor distinction, particularly since as @ndrezn points out the type() function returns the class your object is an instance of, and it fits the pattern of None / NoneType. But if you want to do something else to distinguish the no_update instance from its class that's fine by me, so long as whatever we do makes it clear not to use the class.

from ._get_app import get_app # noqa: F401,E402
from ._get_paths import ( # noqa: F401,E402
get_asset_url,
Expand Down