-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Deprecate empty offsets in get_path_collection_extents #23200
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
Codecov says your addition is never exercised in the tests; so it cannot break the tests 😄 . |
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.
Anyway, I wouldn't know what to test. Passing empty offsets is trivial and almost not worth a test. But maybe we should do that to pin this as intended behavior.
What I meant was that we do not have any tests that relies on being able to pass an empty The problem I think is to know if it ever is useful to pass an empty |
If we are not sure that somebody is relying on the current but ambiguous behavior, we should deprecate it instead of immediately erroring out. That gives people time to object. The issue has been standing since 2013, so there is no rush in turning it into an error. |
Perhaps what this should really return is a null bbox (see discussion at #17115 (comment))? Although we can always change that later if we manage to resolve #17115 at some point. |
Not sure why I put this as draft, but rebased and updated deprecation version. |
PR Summary
Closes #1735
Based on the issue, the outcome of an empty offsets is not well defined. This is now deprecated.
PR Checklist
Tests and Styling
pytest
passes).flake8-docstrings
and runflake8 --docstring-convention=all
).Documentation
doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).