8000 Improve check for bbox by oscargus · Pull Request #27514 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Improve check for bbox #27514

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

Jump to bottom
Merged
merged 3 commits into from
Dec 20, 2023
Merged

Improve check for bbox #27514

merged 3 commits into from
Dec 20, 2023

Conversation

oscargus
Copy link
Member

PR summary

Passing something else than a Transform would lead to an AttributeError, but now the better error message should show up.

PR checklist

QuLogic
QuLogic reviewed Dec 14, 2023
View reviewed changes
@oscargus oscargus marked this pull request as draft December 14, 2023 11:17
@oscargus oscargus marked this pull request as ready for review December 14, 2023 12:57
@jklymak
Copy link
Member
jklymak commented Dec 14, 2023

Doc build failed?
8000

@QuLogic
Copy link
Member
QuLogic commented Dec 14, 2023

Yes, everything failed earlier this morning due to network issues. I've restarted them all.

anntzer
anntzer previously requested changes Dec 14, 2023
View reviewed changes
Copy link
Contributor
@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Seems more natural to just check_isinstance(BboxBase, bbox=bbox) if we want to go that direction, no? (Also consistent with the line immediately after.)
Feel free to dismiss if you disagree.

@oscargus
Copy link
Member Author
oscargus commented Dec 15, 2023 •
edited
Loading

Seems more natural to just check_isinstance(BboxBase, bbox=bbox) if we want to go that direction, no? (Also consistent with the line immediately after.) Feel free to dismiss if you disagree.

Except that a TypeError will be raised. Which on the other hand makes more sense. I can change if there is agreement on this.

Edit: this also raises the question if is_bbox is actually required? If this is changed, it will be unused. So I'll deprecate it as well if I change it.

@oscargus
Copy link
Member Author

I added the change as a second commit. Not sure if I should deprecate it as a property or classproperty though.

QuLogic
QuLogic reviewed Dec 15, 2023
View reviewed changes
@oscargus @QuLogic
Update lib/matplotlib/transforms.py
24a7468 
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@timhoffm timhoffm merged commit 65d6560 into matplotlib:main Dec 20, 2023
@timhoffm timhoffm added this to the v3.9.0 milestone Dec 20, 2023
@oscargus oscargus deleted the bboxcheck branch December 20, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic approved these changes

@jklymak jklymak jklymak approved these changes

@timhoffm timhoffm timhoffm approved these changes

@anntzer anntzer anntzer left review comments

Assignees
No one assigned
Labels
API: argument checking topic: transforms and scales
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.
5 participants
@oscargus @jklymak @QuLogic @anntzer @timhoffm
0