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

Introduce KnobT, fix signature for nuke.zoom and nuke.filename, use N… #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

herronelou
Copy link

Introduce KnobT, fix signature for nuke.zoom and nuke.filename, use NodeT instead of Node most places.

@chadrik
Copy link
Collaborator

chadrik commented Jan 12, 2024

Hi, thanks for the contribution.

There's no need to replace everything with NodeT and KnobT. TypeVars are only required when you want to create a relationship between two or more type annotations. There are some good examples here: https://filipgeppert.com/mypy-advanced-examples.html

NodeT is used for one specific case:

class NodeConstructor(Generic[NodeT]):
    @classmethod
    def __init__(self, *args, **kwargs) -> None: ...
    def __call__(self, *args, **kwargs) -> NodeT: ...
    def __hash__(self) -> Any: ...

class Nodes:
    @classmethod
    def __init__(self, *args, **kwargs) -> None: ...
    def __hash__(self) -> Any: ...
    def __getattr__(self, item) -> NodeConstructor[Node]: ...

    Group : NodeConstructor[Group]
    BackdropNode : NodeConstructor[BackdropNode]
    Gizmo : NodeConstructor[Gizmo]
    Viewer : NodeConstructor[Viewer]

nodes: Nodes

This ensures that a type analyzer knows that nodes.Group.__call__() (i.e. nodes.Group()) returns an instance of Group, and nodes.BackdropNode() returns a BackdropNode, etc.

Can you remove the KnobT and NodeT changes, please, unless there is a relationship that requires a TypeVar? Then I'll review the remaining parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants