-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Don't make VBoxDivider inherit from HBoxDivider. #20069
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
Conversation
While you are fixing all this I don't think any of it works properly with subfigures because it depends on figsize. |
I'm not particularly happy with the style of this. These methods are quite obscure. As private static methods, at least their scope was clear. I find the prefix Suggested minimal improvement: Remove the For future PRs: Please split moving code and renaming variables into separate commits. This is really hard to follow. |
They share some common calculation helpers, but a VBoxDivider is not a specialization of an HBoxDivider, so just move the helpers out to free functions (they're staticmethods or nearly so anyways).
Added some more comments, and split the variable renaming into a second commit. The first one is solely about moving the staticmethods out, now. The alternative would be to have a private common base class to host the staticmethods, but private base classes come with their own problems... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much simpler to review. Thanks for cleaning up. 😄
The alternative would be to have a private common base class to host the staticmethods, but private base classes come with their own problems...
I intentionally did not suggest that 😜
... in internal APIs to always have x/width first, y/height second.
Actually, I further amended the second commit to always put the width first and height second in the internal APIs (instead of having x/y but then height/width). |
They share some common calculation helpers, but a VBoxDivider is not a
specialization of an HBoxDivider, so just move the helpers out to free
functions (they're staticmethods or nearly so anyways).
Also rename "equivalent" to "equal" and "appended" to "summed", which I
think actually express the constraints better, and are shorter too.
Also document their usage, and PEP8-ify a couple of variables.
PR Summary
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).