Warning if handles and labels have a len mismatch

[borgesaugusto]

… when __ len __ exists for both

PR summary

Closes #24050 . Following the discussion there (And in PRs #24061 and #24063) a warning is issued when the handles and labels have __ len __() and there is a mismatch in its value. A test checks whether the warning is raised.

PR checklist

• "closes #0000" is in the body of the PR description to link the related issue
• new and changed code is tested
• Plotting related features are demonstrated in an example
• New Features and API Changes are noted with a directive and release note
• Documentation complies with general and docstring guidelines

[github-actions]

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

[tacaswell]

Is it better do do the hasattr checks or catch (and ignore) the TypeError fro just trying?

[borgesaugusto]

I wondered the same. My reasoning was that since in the previous discussion it was agreed that only a warning should be raised , it made more sense to check for the attiribute rather than to do a try/except.
From a speed pov, I don't think there is going to be any big differences (try/except should be faster when __ len __ exists and viceversa). This stack discussion could also be useful.

With try/except would be something like:

try:
    if len(handles) != len(labels):
        _api.warn_external(f"Mismatched number of handles and labels: "
                               f"len(handles) = {len(handles)} "
                               f"len(labels) = {len(labels)}")
except TypeError:
    pass

vs the current impl

matplotlib/lib/matplotlib/legend.py

Lines 1340 to 1345 in 58a80fd

	if (hasattr(handles, "__len__") and
	hasattr(labels, "__len__") and
	len(handles) != len(labels)):
	_api.warn_external(f"Mismatched number of handles and labels: "
	f"len(handles) = {len(handles)} "
	f"len(labels) = {len(labels)}")

I think using the hasattr() is cleaner, but I don't have a strong opinion about either.

In this file, hasattr is used once before:

matplotlib/lib/matplotlib/legend.py

Lines 1251 to 1257 in 58a80fd

	# support parasite axes:
	if hasattr(ax, 'parasites'):
	for axx in ax.parasites:
	handles_original += [
	*(a for a in axx._children
	if isinstance(a, (Line2D, Patch, Collection, Text))),
	*axx.containers]

All other cases use try/except.

[timhoffm]

+0.1 on the version of the PR.
IMHO here: "explicit is better than implicit" > "it's better to ask for foregiveness than for permission", because TypeError is quite generic.

[ksunden]

I could see an argument for moving it into the if handles and labels: block, which is already the next check.

That should make it so that both are at least iterable (escaping the one or both is None case), though I guess someone could pass e.g. labels=(str(x) for x in itertools.count()), which has no len and is relying on the zip, so still need some guard against unbounded inputs.

[timhoffm]

I think this is clearer. We're not obliged to jump extra hoops for user-friendly length checks if the user passes an iterable without a len implementation (which should be quite rare anyway).

[rcomer]

Thanks @borgesaugusto, I think this is good to go. Are you confident to squash the commits down to one? If not we can handle that in the merge.

To squash, you can do an interactive rebase. I strongly recommend saving a backup copy of your branch first if you have not done this (much) before.

[rcomer]

Thanks @borgesaugusto, this is in.