Skip to content

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

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

oscargus
Copy link
Member

PR Summary

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@@ -114,7 +114,7 @@ def _get_packed_offsets(widths, total, sep, mode="fixed"):
offsets = offsets_[:-1]
return total, offsets

elif mode == "equal":
else: # "equal"
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

@oscargus oscargus marked this pull request as draft November 18, 2022 10:21
@oscargus oscargus marked this pull request as ready for review December 8, 2022 09:05
@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label Dec 14, 2022
@oscargus oscargus force-pushed the offsetboxtests branch 2 times, most recently from 7f28431 to 79247c1 Compare December 28, 2022 10:34
@oscargus
Copy link
Member Author

I removed the discussion point since that was a simple cleanup and will open an issue instead. This is now just tests.

Copy link
Member

@jklymak jklymak left a 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!

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.

4 participants