8000 #24050 No error was thrown even number of handles mismatched labels by tusharkulkarni008 · Pull Request #24061 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

#24050 No error was thrown even number of handles mismatched labels #24061

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

#24050 No error was thrown even number of handles mismatched labels #24061

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/matplotlib/legend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1260,6 +1260,8 @@ def _parse_legend_args(axs, *args, handles=None, labels=None, **kwargs):
elif len(args) >= 2:
handles, labels = args[:2]
extra_args = args[2:]
elif len(handles)!= len(labels):
Copy link
Member

Choose a reason for hiding this comment

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

This condition will never be evaluated because the one of the other conditionals in this if/elif chain will always be True first.

Copy link
Author

Choose a reason for hiding this comment

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

Yes I see that now.
An entirely new block would do the trick or we should rethink like you said. This issue was tagged as good first issue so and solution seemed pretty straightforward at the time.will analyslze along your directives

Copy link

Choose a reason for hiding this comment

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

I think an entirely new block should be enough. This conditional statement (L1222:L1262) is to assign values to labels and handles. Checking the validity of handles and labels should come in a new conditional statement after that.

raise ValueError("fewer handles than labels or vice versa")
Copy link

Choose a reason for hiding this comment

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

It would be helpful to provide a better error message. Maybe include the length of handles and labels to make it easier for the developer to understand what they did wrong.

Suggested change
raise ValueError("fewer handles than labels or vice versa")
raise ValueError(f"Mismatched number of handles and labels. len(handles)={len(handles)} and len(labels)={len(lables)}.")

Copy link

Choose a reason for hiding this comment

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

Sorry didn't mean to set myself as a reviewer. I just wanted to give my comments to @tusharkulkarni008 since it is their first PR ever.

Copy link
Member

Choose a reason for hiding this comment

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

@aqeelat the developer guide encourages you to review 🙂
https://matplotlib.org/stable/devel/triage.html#working-on-prs-to-help-review

Copy link
Author

Choose a reason for hiding this comment

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

Sorry didn't mean to set myself as a reviewer. I just wanted to give my comments to @tusharkulkarni008 since it is their first PR ever.

Yes @aqeelat this is my first PR ever and earlier in this conversation other senior member pointed out some important in points in very detailed explanation. Since then I am observing PRs other people make to gain some footing to make more meaningful PRs.
Certainly your error message is more consistent with raised issue


else:
raise TypeError('Invalid arguments to legend.')
Expand Down
0