Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implements a different approach for __repr__ #78
Changes from 6 commits
4f335ed
5d8a033
eb98987
b482bef
019abe4
73f1aef
8da17d3
70857de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the reason why I added a try/except. running the following causes an API 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.We could remove the try/except and deal with this as a separate issue though, which is probably a cleaner strategy.
There was a problem hiding this comment.
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.
cmdx/cmdx.py
Line 3868 in 344aed0
It would save on the use of try/except too.
Did we also have a test for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I thought of doing it that way as well, but thought it looked less elegant. But sounds good, let me implement that.
We do not, let me add it in.
On another note:
If this is not the case anymore, and we never expect
str
to fail, would it be ok to expect that it can returnNone
? 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.:A third option, albeit more complex, could be to have it return a dictionary, with the child attribute names as keys for instance.
There was a problem hiding this comment.
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.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Scratch that. We can't return something that isn't a string type from
__str__
. That raises aTypeError
, 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 aboveThere was a problem hiding this comment.
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: