Skip to content

Improve check for bbox#27514

Merged
timhoffm merged 3 commits intomatplotlib:mainfrom
oscargus:bboxcheck
Dec 20, 2023
Merged

Improve check for bbox#27514
timhoffm merged 3 commits intomatplotlib:mainfrom
oscargus:bboxcheck

Conversation

@oscargus
Copy link
Copy Markdown
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

@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
Copy Markdown
Member

jklymak commented Dec 14, 2023

Doc build failed?

@QuLogic
Copy link
Copy Markdown
Member

QuLogic commented Dec 14, 2023

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

Copy link
Copy Markdown
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
Copy Markdown
Member Author

oscargus commented Dec 15, 2023

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
Copy Markdown
Member Author

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

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants