Type annotations

[Muream]

I've started working on making type stubs for cmdx.

I ended up going the stubs way as I used a few features that wouldn't work in python 2 without the typing and typing-extensions libraries.

I've only added two commits so far, the first one is the raw stubs from stubgen, not too interesting
Have a look at the 2nd one to see what I did exactly.

most things should be pretty self explanatory but a few nice QoL improvements are already standing out, even if you're not into static typing:

1. Your editor is aware that Node.__getitem__ returns a plug so you can have nice completion and all that when retrieving a plug
   
2. I've added @overload decorators on a couple methods like Node.connections which let me assign different return types to different arguments.
   In this case based on the plugs and connections arguments, we get different return types and your editor is aware of that

for node in node.connections(plugs=False, connections=False):  # Generator[Node]
#   ^---- This is a Node
    pass

for src_node, dest_node in node.connections(plugs=False, connections=True):  # Generator[Tuple[Node, Node]]
#   ^---------^------ These are Nodes from a tuple
    pass

for plug in node.connections(plugs=True, connections=False):  # Generator[Plug]
#   ^---- This is a Plug
    pass

for src_plug, dest_plug in node.connections(plugs=True, connections=True):  # Generator[Tuple[Plug, Plug]]
#   ^---------^------ These are Plugs from a tuple
    pass

Couple questions:

1. is Node.data() ever used anywhere? it seems to access _data which is never set
2. Would renaming _AbstractAttribute to Attribute and making it an actual abstract class be ok with you?
   It feels weird to include it in the signature of the public API considering that it's currently private

[mottosso]

Nice one.

Your editor is aware that Node.getitem returns a plug so you can have nice completion and all that when retrieving a plug

This is interesting, because in your screenshot, it shows you the PEP8 versions of the name. Those are technically aliases, and not the actual names of the class, which are mixedCase, like the Maya API. I personally only use the snake_case names in my code, but it's not trivial to make those the only names, since a lot of the classes are subclasses that inherit their mixedCase names.

So, I wonder if the auto-complete shows both versions? 🤔 Should it?

is Node.data() ever used anywhere? it seems to access _data which is never set

This is "metadata". A transient dictionary you can use to attach data that persists with the node, but isn't saved with the scene.

• https://github.com/mottosso/cmdx#metadata

Would renaming _AbstractAttribute to Attribute and making it an actual abstract class be ok with you? It feels weird to include it in the signature of the public API considering that it's currently private

What is an actual abstract class? 🤔

[Muream]

So, I wonder if the auto-complete shows both versions?

It's actually showing both, the camelCase one was just lower down the completion cause it was sorted alphabetically.
The snake_case one shows up as a variable in the list because it's an alias but we still get the signature and the documentation

Should it?

I'm fine with showing both, at least for now
I might be able to define one or the other based on ENABLE_PEP8 but I'm not sure how the stub file could be aware of its actual value unless I make it read an environment variable in the stubs.

What is an actual abstract class? 🤔

An abstract class is a class that can't be instantiated directly and has to be inherited from. They mostly serve as defining a common interface that all subclasses must implement

from abc import ABC, abstractmethod


class Attribute(ABC):
    @abstractmethod
    def read(self, data):
        pass

class Double(Attribute):
    def read(self, data):
        return 1.0

# This works:
attr = Double()

# But this fails:
# TypeError: Can't instantiate abstract class Attribute with abstract methods read
attr = Attribute()

Speaking of attributes:

• I failed to understand why _AbstractAttribute was a dict subclass?
• The fact that its __new__ returns either an instance or a tuple with the type and kwargs based on whether we pass only keyword arguments or not was also confusing.
  I understand that it's to allow node["attr"] = Double() without specifying a name in the attribute's constructor but it feels weird to instantiate a type and not get an instance of that type back
  This prevents me from annotating Node.__setitem__ clearly like this

# This snippets contains stubs for creating attributes only, not setting them.

class Node:
    # Ideal type annotation
    def __setitem__(self, key: str, value: _AbstractAttribute) -> None: ...

    # current type annotation
    def __setitem__(self, key: str, value: Tuple[Type[_AbstractAttribute], Dict[str, Any]) -> None: ...

I am absolutely not expecting you to agree with all my suggestions btw, I fully appreciate that some of the changes I'm going to suggest are going to turn out to radical to actually be implemented haha

[mottosso]

It's actually showing both

Ok, sounds reasonable.

An abstract class is a class that can't be instantiated directly and has to be inherited from

Ok, but why? What is it about the _AbstractAttribute class that would benefit from that? Would they not show up in your auto-complete is that the problem? That they appear first, due to alphabetical sorting?

I failed to understand why _AbstractAttribute was a dict subclass?
I understand that it's to allow node["attr"] = Double() without specifying a name in the attribute's constructor

That's exactly it.

It's not much used, at least I don't use it. Because it doesn't jive with undo/redo. So if it's a problem, we could probably deprecate that convenience. Initially it came from the earliest version of cmdx, where there was no classes, only a plain dictionaries of plain data. I liked the simplicity of that, but needed a method or two attached to it, until eventually it became the mostly-class-looking thing that it is today.

[Muream]

Ok, but why? What is it about the _AbstractAttribute class that would benefit from that? Would they not show up in your auto-complete is that the problem? That they appear first, due to alphabetical sorting?

My thought process was:

• It feels weird to have a private member in a bunch of function definitions: we should remove the _ prefix
• But then it might confuse users that the class is meant to be used directly: We should make it an actual Abstract class with some @abstractmethod.
  This is a great way to communicate this and inform users what methods they should re-implement through your code without having to document it.
• Since the class is Abstract already, it doesn't really need the Abstract prefix anymore: it can be named Attribute now

This is not needed right, it just feels cleaner to me

It's not much used, at least I don't use it. Because it doesn't jive with undo/redo. So if it's a problem, we could probably deprecate that convenience. Initially it came from the earliest version of cmdx, where there was no classes, only a plain dictionaries of plain data. I liked the simplicity of that, but needed a method or two attached to it, until eventually it became the mostly-class-looking thing that it is today.

Ahh I gotcha, you use node.add_attr(Double("attr_name")) instead then?
I don't think I should refactor anything that touches on functionality on this PR, even that Abstract method thing

Out of curiosity, do you have a strategy for deprecating features or do you just remove things?

[mottosso]

It feels weird to have a private member in a bunch of function definitions: we should remove the _ prefix

Is this something visual? Something in your IDE? I'm trying to figure out what's weird about it, I had almost forgotten it even existed as it's just a baseclass. It's private with an underscore to highlight that you aren't meant to instantiate it which I don't expect anyone would even try. We could make it "abstract" and prevent instantiation further, but I just don't see what it would change?

I'm inclined to keep things as they are, unless you can convince me this isn't just bikeshedding. :)

you use node.add_attr(Double("attr_name")) instead then?

Actually not even that, because that is also not undoable. My use is primarily interactive, so the modifiers are the way to go.

with cmdx.DagModifier() as mod:
  node = mod.create_node("transform")
  mod.add_attr(node, cmdx.Double("attrName", default=1.0))

For plug-ins, that's a different story and was the original usecase for cmdx, for which the former syntax is the most readable I think.

node["someDouble"] = cmdx.Double(default=1)
node["someLong"] = cmdx.Long()

Out of curiosity, do you have a strategy for deprecating features or do you just remove things?

Nothing is removed and there is no deprecation.

We can nudge and recommend alternatives, but backwards compatibility is of utmost importance. Anything removed must have an equivalent wrapper or alias to the same name and arguments. Think of how software written for Windows 95 still works on Windows 11 and you get the idea. :)