-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
ENH: include property name in artist AttributeError #28573
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
Conversation
with pytest.raises(AttributeError) as exec_info: | ||
ax.voxels(filled=filled, x=x, y=y, z=z) | ||
assert exec_info.value.name == 'x' |
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 was the only test I found that covers the changed code. Should I add something new or is this enough?
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.
Take or leave my optional suggestions. I don’t think this needs additional tests.
@@ -1501,8 +1501,9 @@ def test_calling_conventions(self): | |||
ax.voxels(x, y) | |||
# x, y, z are positional only - this passes them on as attributes of | |||
# Poly3DCollection | |||
with pytest.raises(AttributeError): | |||
with pytest.raises(AttributeError) as exec_info: |
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.
Maybe add a match
parameter to check that the name is in the message.
@@ -1203,7 +1203,8 @@ def _update_props(self, props, errfmt): | |||
func = getattr(self, f"set_{k}", None) | |||
if not callable(func): | |||
raise AttributeError( | |||
errfmt.format(cls=type(self), prop_name=k)) | |||
errfmt.format(cls=type(self), prop_name=k), |
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.
The docstring could mention that formatting happens via named placeholders {cls}
and {prop_name}
.
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.
I think you should add a what's new cause that's probably the easiest way to tell folks "hey better error messages" and you've basically already written it in the PR body, so I'm gonna leave this for you to merge if you wanna add one (or not).
Added a whatsnew. Is there a better directive to use for the output block? |
PR summary
Closes #27878
Add a
name
to the exception that is raised when an invalid keyword parameter is passed. This helps downstream packages who wrap our functions with their exception handling:We could also add the artist type as the
obj
attribute, or we could wait till someone asks for that.Should this get a whatsnew entry?
PR checklist