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

expose NoUpdate #2800

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

expose NoUpdate #2800

wants to merge 4 commits into from

Conversation

2Ryan09
Copy link

@2Ryan09 2Ryan09 commented Mar 16, 2024

Expose NoUpdate at top-level dash.

The primary application is for type hinting, allowing for comprehensive type checking of dash callbacks.

See: #2799

Contributor Checklist

  • Expose NoUpdate from dash._callback
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

dash/__init__.py Outdated
@@ -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

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.

@2Ryan09
Copy link
Author

2Ryan09 commented Mar 21, 2024

I just pushed the minimal possible change in my mind: renaming NoUpdate to NoUpdateType and exposing it.

This does not implement the behavior @ndrezn mentions as it will still be of type dash._callback.NoUpdateType, but is there value in it being dash.NoUpdate? It's the only class as the other two imported into dash are functions, so maybe NoUpdate is just declared in the wrong module?

Given NoUpdate can be entirely described by {"_dash_no_update": "_dash_no_update"} (see context in #2800), can it not somehow inherit from Dict or TypedDict? This could make its behavior much more predictable and pythonic.

@T4rk1n
Copy link
Contributor

T4rk1n commented Mar 21, 2024

Given NoUpdate can be entirely described by {"_dash_no_update": "_dash_no_update"} (see context in #2800), can it not somehow inherit from Dict or TypedDict? This could make its behavior much more predictable and pythonic.

It does not equate to a typed dict, it's a singleton object, the dict representation is for internal use.

@2Ryan09
Copy link
Author

2Ryan09 commented Apr 10, 2024

In an attempt to keep the implementation of this PR at a minimum and, if I'm interpreting the conversation correctly up until now, is there any objection to the current state of this PR?

Copy link
Member

@ndrezn ndrezn left a comment

Choose a reason for hiding this comment

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

💃 for me -- thanks for making those changes! Should be quite useful for improving type signatures in Dash apps. @T4rk1n , any lingering comments from you?

@T4rk1n
Copy link
Contributor

T4rk1n commented Apr 11, 2024

any lingering comments from you?

I'd rather we fix the cases where using NoUpdate would be creating an error instead of renaming the variable in hope it won't be used.
I think the only case is actually just returning the class like this:

from dash import NoUpdate

def cb(...):
    return NoUpdate

intead of return NoUpdate()

Think we can add the check like this:

    @staticmethod
    def is_no_update(obj):
        return obj is NoUpdate or isinstance(obj, NoUpdate) or (
            isinstance(obj, dict) and obj == {"_dash_no_update": "_dash_no_update"}
        )

Then change all our checks to NoUpdate.is_no_update(obj):

@2Ryan09
Copy link
Author

2Ryan09 commented Apr 11, 2024

any lingering comments from you?

I'd rather we fix the cases where using NoUpdate would be creating an error instead of renaming the variable in hope it won't be used. I think the only case is actually just returning the class like this:

from dash import NoUpdate

def cb(...):
    return NoUpdate

intead of return NoUpdate()

Think we can add the check like this:

    @staticmethod
    def is_no_update(obj):
        return obj is NoUpdate or isinstance(obj, NoUpdate) or (
            isinstance(obj, dict) and obj == {"_dash_no_update": "_dash_no_update"}
        )

Then change all our checks to NoUpdate.is_no_update(obj):

I agree with this. Specifically, return NoUpdate seems like a more desirable behavior than return no_update.

However, such a change seems adjacent to the topic of this PR which is to simply expose the value. I am more than happy to open another PR suggesting this change.

@gvwilson gvwilson self-assigned this Jul 25, 2024
@gvwilson gvwilson removed their assignment Aug 2, 2024
@gvwilson gvwilson added feature something new P2 considered for next cycle labels Aug 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants