Skip to content

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

Merged
merged 1 commit into from
Jul 14, 2024

Conversation

rcomer
Copy link
Member

@rcomer rcomer commented Jul 14, 2024

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:

import matplotlib.pyplot as plt

def wobbly_plot(args, **kwargs):
    w = kwargs.pop('wobble_factor', None)
    
    try:
        plt.plot(args, **kwargs)
    except AttributeError as e:
        raise AttributeError(f'wobbly_plot does not take parameter {e.name}') from e
    
    
wobbly_plot([0, 1], wibble_factor=5)
<snip>
AttributeError: Line2D.set() got an unexpected keyword argument 'wibble_factor'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/ruth/python/wobble_plot.py", line 12, in <module>
    wobbly_plot([0, 1], wibble_factor=5)
  File "/home/ruth/python/wobble_plot.py", line 9, in wobbly_plot
    raise AttributeError(f'Wobbly_plot does not take parameter {e.name}') from e
AttributeError: wobbly_plot does not take parameter wibble_factor

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

@rcomer rcomer added this to the v3.10.0 milestone Jul 14, 2024
Comment on lines 1504 to 1506
with pytest.raises(AttributeError) as exec_info:
ax.voxels(filled=filled, x=x, y=y, z=z)
assert exec_info.value.name == 'x'
Copy link
Member Author

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?

Copy link
Member

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:
Copy link
Member

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),
Copy link
Member

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}.

Copy link
Member

@story645 story645 left a 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).

@rcomer
Copy link
Member Author

rcomer commented Jul 14, 2024

Added a whatsnew. Is there a better directive to use for the output block?

@story645
Copy link
Member

story645 commented Jul 14, 2024

Is there a better directive to use for the output block?

@QuLogic or @ksunden (both at table w/ me) said looks fine and that they'd clean it up as part of release if they have opinions.

@story645 story645 merged commit 28eae9b into matplotlib:main Jul 14, 2024
40 checks passed
@rcomer rcomer deleted the attr-error branch July 15, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants