-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Minor cleanup and add test for offsetbox #24486
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
lib/matplotlib/offsetbox.py
Outdated
@@ -114,7 +114,7 @@ def _get_packed_offsets(widths, total, sep, mode="fixed"): | |||
offsets = offsets_[:-1] | |||
return total, offsets | |||
|
|||
elif mode == "equal": | |||
else: # "equal" |
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.
I don't know whether this is better. On the one hand, you don't need it. On the other hand, keeping the symmetry and stating the value explicitly (not only as a comment also has some appeal.
Execution-wise it makes no difference. There is however a difference in case the if-else-chain gets out of sync with the check_in_list
above. When using else
other values will be treated like "equal". When using elif mode
other values don't get that execution block and the function will continue. Both leads to an incorrect result, but the elif
solution will most likely blow up in one way or the other, which is preferable IMHO.
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.
The reason was to be able to get full test coverage. Now I tried another pattern that I've seen used: placing the check_in_list in a final else-clause, so more used to raise an error than to check the arguments beforehand.
I guess that in general there should be a preferred way, but it seems like there isn't.
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.
Personally, I don't worry too much about full test coverage. It's onerous in some cases, and full test coverage does not mean your code is working correctly. Test coverage is only a weak indicator and we should be pragmatic about it.
I agree there should be a preferred way. Maybe we should briefly discuss this in a dev call. Unfortunately, there is no really good solution. You always have the duplication between the if/else cases and the values in check_in_list with the danger of being out of sync. For readability, I prefer check_in_list
at the top, because that limits the scope what can happen in the code afterwards and thus reduces mental load.
fb4f7eb
to
31c891f
Compare
31c891f
to
e04a09b
Compare
e04a09b
to
329b28c
Compare
7f28431
to
79247c1
Compare
I removed the discussion point since that was a simple cleanup and will open an issue instead. This is now just tests. |
79247c1
to
f9e9c34
Compare
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.
Seems like a useful set of tests to add. Thanks!
PR Summary
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst