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

Implements a different approach for __repr__ #78

Merged
merged 8 commits into from
Feb 27, 2024

Conversation

chelloiaco
Copy link
Contributor

@chelloiaco chelloiaco commented Feb 17, 2024

Description of Changes

Expanding on #20 , this changes repr to give a clear representation of what the object is, preventing the user from assuming it is a string. The goal, while not as forgiving, is to avoid unpredictable behavior by making the user having to explicitly request the name of a node via .name(), its path via .path(), and the value of one of its attributes via .read().

Note: I opted to have __str__ be different from __repr__, that way we still have the ability to str(node) and get the previous behavior, as seen by the changes done on tests.py. I'm not certain if this would be confusing or unpredictable, but let me know.

Testing done

As expected, I had to change the doctests to utilize .name(), .path() or .read() accordingly. After the changes, those pass without failures:

===Flaky Test Report===

test_createNode_performance passed 1 out of the required 3 times. Running test again until it passes 3 times.
test_createNode_performance passed 2 out of the required 3 times. Running test again until it passes 3 times.
test_createNode_performance passed 3 out of the required 3 times. Success!
test_rouge_mode passed 1 out of the required 3 times. Running test again until it passes 3 times.
test_rouge_mode passed 2 out of the required 3 times. Running test again until it passes 3 times.
test_rouge_mode passed 3 out of the required 3 times. Success!

===End Flaky Test Report===
----------------------------------------------------------------------
Ran 153 tests in 1.861s

OK
Skipping coveralls

cmdx.py Outdated Show resolved Hide resolved
cmdx.py Show resolved Hide resolved
cmdx.py Outdated Show resolved Hide resolved
@chelloiaco
Copy link
Contributor Author

chelloiaco commented Feb 19, 2024

There we go, now I can actually see what these look like. What do you think about:

# Before
<cmdx.DagNode : '|node'>

# After
<cmdx.DagNode("|node")>

That way, it'll be more familiar and copy-pasteable as real code too.

I did think at one point to use parenthesis similarly to your suggestion, but decided not to go with it exactly because it started to look like executable code.

The issue is that there isn't a matching initializer for DagNode (or Node) that takes in a string name. I'm thinking this could potentially be confusing since it wouldn't really be copy-pasteable. However, it would be easy enough to implement if we wanted to go that route.

That said, it gets a bit more complicated for Plug nodes if we want copy-pasteable code. Considering node name initialization is supported, it wouldn't be helpful to have:

>>> node["translate"]
<cmdx.Plug("|node", "translate") = (0.0, 0.0, 0.0)>

This is because assignment operator (=) to a function call is syntactically invalid in Python and therefore the provided code snippet wouldn't be executable. A valid solution would be:

>>> node["translate"]
<cmdx.Plug("|node", "translate").write((0.0, 0.0, 0.0))>

But, while it shows what could be executable code, this starts becoming cryptic and isn't a concise representation of the object.

Let me know your thoughts. What I used was really just an initial suggestion to get the idea across, by no means it's the best solution!

@mottosso
Copy link
Owner

Hmm, a lot of minutia here.

I had a look at how PyMEL does it; I've never noticed it before, which is a good sign.

from pymel import core as pm
t = pm.createNode("transform", name="myTransform")
print(repr(t))
# nt.Transform('myTransform')

It looks like an unrelated instance, due to the nt namespace, which is unexpected. It also does not include hierarchy in the name, which I prefer. I would expect that if a user wanted to know the path, rather than name, they would not print the node but ask for its .path(). Displaying the type and name when printing sounds more intuitive to me.

For plugs..

print(repr(t.translateX))
# Attribute('myTransform.translateX')

Now there is no namespace at all, which is also unexpected. There is also no value, which is unexpected also; why print a value if it isn't going to give you the value?

But I do like the presence of the object and attribute name, so let's keep that.

How about.

>>> node
cmdx.DagNode("myTransform")
>>> node
cmdx.Node("multMatrix1")
>>> node["translateX"]
cmdx.Plug("myTransform", "translateX") ==  0.0
>>> node["translate"]
cmdx.Plug("myTransform", "translate") == (0.0, 0.0, 0.0)

Where we:

  1. Use double-quoted
  2. Skip the <> prefix/suffix
  3. Skip the hierarchy of the dag node
  4. Use == to denote the value of a plug

Later on, we can discuss whether or not to make this actually executable; for now, it reads well I think.

@chelloiaco
Copy link
Contributor Author

>>> node
cmdx.DagNode("myTransform")
>>> node
cmdx.Node("multMatrix1")
>>> node["translateX"]
cmdx.Plug("myTransform", "translateX") ==  0.0
>>> node["translate"]
cmdx.Plug("myTransform", "translate") == (0.0, 0.0, 0.0)

I've changed the reprs to display this way and changed the tests accordingly. All seems to be working good and passing the tests.

There is one thing that I ran into though, which is due to how .read() works:

>>> _ = cmds.file(new=1, f=1)
>>> sphere_a = cmds.polySphere(ch=False)[0]
>>> sphere_b = cmds.polySphere(ch=False)[0]
>>> bs = cmds.blendShape(sphere_a, sphere_b)[0]
>>> bs = cmdx.ls(bs)[0]
>>> bs
cmdx.Node("blendShape1")
>>> bs['input']
cmdx.Plug("blendShape1", "input") == (None, 0, '*')

What's happening here is that its read()ing the values for 'input[0].inputGeometry', input[0].groupId' and 'input[0].componentTagExpression'. Might be an edge-case but looks kinda weird. A solution would be to add checks to only read() if the type is worth reading. But, then we would probably be over-complicating the repr


PS: I feel like I might've opened a can of worms, but running bs['inputTarget'] causes an API failure when it tries to get 'targetBindMatrix'. I might create a separate issue on it so we keep on topic here.

@mottosso
Copy link
Owner

cmdx.Plug("blendShape1", "input") == (None, 0, '*')

You mean because input is a compound attribute? Maybe we could limit value printing to simple types - float, double, bool and string?

Other than that, I think it looks good!

cmdx.py Outdated
@@ -2674,12 +2674,16 @@ def __str__(self):
return str(self.read())

def __repr__(self):
cls_name = '{}.{}'.format(__name__, self.__class__.__name__)
if self._mplug.attribute().apiType() == om.MFn.kCompoundAttribute:
Copy link
Owner

@mottosso mottosso Feb 22, 2024

Choose a reason for hiding this comment

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

I would go the opposite route; include rather than exclude. There are lots of types, and if we aren't going to test them all then we should only include the ones we know work. I expect printing meshes and MObject types to also be problematic. Only the float, double, bool and string are interesting, possibly enum too.

Copy link
Contributor Author

@chelloiaco chelloiaco Feb 22, 2024

Choose a reason for hiding this comment

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

Instead of adding more checks into the repr, I thought, since self.typeClass() is doing all the heavy lifting, I should use that info instead.

I had to add a couple more types since a lot of these are types of some basic queries like node['transform'] or node['rotateX'] but this is what I came up with:

...
valid_types = (Angle, Boolean, Distance, Double, Double3, Enum, Float,
               Integer, Long, Matrix, String)
if self.typeClass() not in valid_types:
    return '{}("{}", "{}")'.format(cls_name,
                                    self.node().name(),
                                    self.name())
...

Which worked, with the exception of when the type has not been implemented, since self.typeClass() raises an error. This happens specifically with the following doctest on line 4180:

>>> _ = cmds.file(new=True, force=True)
>>> a = createNode("transform", name="A")
>>> b = createNode("multDoubleLinear", name="B")
>>> a["ihi"] << b["ihi"]
>>> a["ihi"].connection() == b
True
>>> b["ihi"].connection() == a
True
>>> a["ihi"]  # Error here

That is because isHistoricallyInteresting is a kNumericAttribute of kByte NumericData, which isn't supported in self.typeClass()

I could add a try except in the repr like so and then not read() the value:

...
typ_class = None
try:
    typ_class = self.typeClass()
except TypeError:
    pass

if typ_class not in valid_types:
...

But then the doctest above fails for a different reason:

# expected
cmdx.Plug("A", "isHistoricallyInteresting") == 2

# actual repr return
cmdx.Plug("A", "isHistoricallyInteresting")

Would it be ok to remove the value return from that test? Also, in regards to having a try except in the __repr__, I'm not so sure about that idea either, but inevitably there will be cases where the attribute isn’t implemented in typeClass, so it has to account for that.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, true.. Choices choices.

In that case, I think we can just put whatever comes out of str(plug) after the ==. It can be a try/except; if it fails, we do not include the ==. It's a __repr__ after all, for debugging. Not performance sensitive or in need to be exact. Then we could potentially work, separately, on making more values print nicely via str(plug)

cmdx.py Outdated Show resolved Hide resolved
cmdx.py Outdated
Comment on lines 2689 to 2695
try:
if self.typeClass() == String:
# Add surrounding quotes, indicating it is a string
read_result = '"{}"'.format(read_result)
except TypeError:
pass
msg += ' == {}'.format(read_result)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add this in order to avoid cases where it shows a valid value, but of an empty string:

>>> myNode["myStr"]
cmdx.Plug("myNode", "myStr") == 

@chelloiaco
Copy link
Contributor Author

There is one thing that I ran into though, which is due to how .read() works:

>>> _ = cmds.file(new=1, f=1)
>>> sphere_a = cmds.polySphere(ch=False)[0]
>>> sphere_b = cmds.polySphere(ch=False)[0]
>>> bs = cmds.blendShape(sphere_a, sphere_b)[0]
>>> bs = cmdx.ls(bs)[0]
>>> bs
cmdx.Node("blendShape1")
>>> bs['input']
cmdx.Plug("blendShape1", "input") == (None, 0, '*')

This new commit brings back the behavior above for compound attributes (if they don't happen to raise an error). But, I do like the idea of having __str__ (or perhaps self.read?) deal with it. Two (three?) birds one stone.

cmdx.py Outdated
return str(self.read())
try:
# Delegate the value reading to __str__
read_result = str(self)
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmmm, now str() is a call I would never expect to fail, and if it does then I'd consider that a bug. We can expect it will never fail, and not bother with a valid = True.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: I feel like I might've opened a can of worms, but running bs['inputTarget'] causes an API failure when it tries to get 'targetBindMatrix'. I might create a separate issue on it so we keep on topic here.

This is the reason why I added a try/except. running the following causes an API failure:

>>> _ = cmds.file(new=1, f=1)
>>> sphere_a = cmds.polySphere(ch=False)[0]
>>> sphere_b = cmds.polySphere(ch=False)[0]
>>> bs = cmds.blendShape(sphere_a, sphere_b)[0]
>>> bs = cmdx.ls(bs)[0]
>>> bs["inputTarget"].read()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "D:\repos\cmdx\cmdx.py", line 4044, in read

  File "D:\repos\cmdx\cmdx.py", line 5076, in _plug_to_python
    #
  File "D:\repos\cmdx\cmdx.py", line 5094, in _plug_to_python
    )
  File "D:\repos\cmdx\cmdx.py", line 5094, in <genexpr>
    )
  File "D:\repos\cmdx\cmdx.py", line 5076, in _plug_to_python
    #
  File "D:\repos\cmdx\cmdx.py", line 5094, in _plug_to_python
    )
  File "D:\repos\cmdx\cmdx.py", line 5094, in <genexpr>
    )
  File "D:\repos\cmdx\cmdx.py", line 5119, in _plug_to_python
    # E.g. transform["worldMatrix"][0]
RuntimeError: (kFailure): Unexpected Internal Failure

Might be an edge case, but that was the reason why I added tat in.

I did some digging and it could be because the attribute which it fails on (blendshape1.inputTarget[0].inputTargetGroup[0].targetBindMatrix) has its value being cached. It is just a theory at this point though, I haven't properly diagnosed this.

# Isolated API fail:
>>> bs['inputTarget'][0][0][0][4]._mplug.asMObject()             
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: (kFailure): Unexpected Internal Failure

>>> bs['inputTarget'][0][0][0][4]._mplug.isCaching
True

We could remove the try/except and deal with this as a separate issue though, which is probably a cleaner strategy.

cmdx.py Outdated
self.name())
if valid:
try:
if self.typeClass() == String:
Copy link
Owner

Choose a reason for hiding this comment

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

Since we've established that typeClass isn't relevant for all types, like kByte, how about we use Maya's apiType here instead? Especially since we're only dealing with a single type.

It would save on the use of try/except too.

Did we also have a test for this case?

Copy link
Contributor Author

@chelloiaco chelloiaco Feb 23, 2024

Choose a reason for hiding this comment

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

Since we've established that typeClass isn't relevant for all types, like kByte, how about we use Maya's apiType here instead? Especially since we're only dealing with a single type.

Yeah, I thought of doing it that way as well, but thought it looked less elegant. But sounds good, let me implement that.

Did we also have a test for this case?

We do not, let me add it in.


On another note:

I think we can just put whatever comes out of str(plug) after the ==. It can be a try/except; if it fails, we do not include the ==.

If this is not the case anymore, and we never expect str to fail, would it be ok to expect that it can return None? In the case of a Compound attribute for instance? That way I can add that check to include == or not.

This might be getting a little too much into read() functionality for this PR but, perhaps more useful, instead have it return "Compound" or something of the sort? e.g.:

>>> myNode["myCompoundAttr"]
cmdx.Plug("myNode", myCompoundAttr") == Compound

A third option, albeit more complex, could be to have it return a dictionary, with the child attribute names as keys for instance.

>>> myNode["myCompoundAttr"]
cmdx.Plug("myNode", myCompoundAttr") == {"childA": 1.0, "childB": "Some string value.", "childC": None}

Copy link
Owner

Choose a reason for hiding this comment

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

If this is not the case anymore, and we never expect str to fail, would it be ok to expect that it can return None? In the case of a Compound attribute for instance? That way I can add that check to include == or not.

Sure, let's try it.

A third option, albeit more complex, could be to have it return a dictionary, with the child attribute names as keys for instance.

I mean, this would be supremely useful. But I'm uncertain how stable we can make this.. Maybe let's start small, and add this later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let's try it

Scratch that. We can't return something that isn't a string type from __str__. That raises a TypeError, silly me.

I think I was able to think of a decent solution within __repr__, so we're not messing with __str__ in this PR. pushing a commit now which includes this and the above

self.node().name(),
self.name())

if not all((self.isCompound, self.isArray)):
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mean any()? This means it can be an array, and it can be a compound, but it cannot be both at the same time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

all is correct. any would exclude any attribute that isn’t an array or compound.

The idea is that it can display an array (in the case of worldMatrix, for example) and a compound (in the case of translate). But, when dealing with a Compound that is an Array, that’s when it seems to start to become unclear what sort of data will be displayed, namely the example of bs['input'] mentioned previously in this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

But, when dealing with a Compound that is an Array, that’s when it seems to start to become unclear what sort of data will be displayed, namely the example of bs['input'] mentioned previously in this PR

Ahaa, yes in that case. Good. Then this looks good I think! Well done, let's get this show on the road! 👏

@mottosso mottosso marked this pull request as ready for review February 27, 2024 16:51
@mottosso mottosso merged commit 25293da into mottosso:master Feb 27, 2024
8 checks passed
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