Update shift() for issue #1145 #1194
Conversation
added check_imaginary parameter to the function
| @@ -492,7 +492,7 @@ def range( | |||
|
|
|||
| values = [getattr(current, f) for f in cls._ATTRS] | |||
| current = cls(*values, tzinfo=tzinfo).shift( # type: ignore[misc] | |||
| **{frame_relative: relative_steps} | |||
| check_imaginary=True, **{frame_relative: relative_steps} | |||
There was a problem hiding this comment.
Mind explaining why we want to force usage of check imaginary here rather than making it an argument?
There was a problem hiding this comment.
To bypass unnecessary checks when performance matters and they know they won't encounter DST transitions.
The default behavior of check_imaginary=True ensures that users get correct and valid results by default. If a user deliberately wants to skip the check for performance reasons or because they're confident it isn't needed, they can do so by setting the parameter to False.
There was a problem hiding this comment.
Ah my bad, I now see that this was added as true in range and dehumanize shift usages to maintain backwards compatibility right?
|
@jadchaar If all the requirements are met, can you please approve it? |
|
Could you please add a Hacktoberfest accepted label to this, will be helpful |
Pull Request Checklist
Thank you for taking the time to improve Arrow! Before submitting your pull request, please check all appropriate boxes:
toxormake testto find out!).tox -e lintormake lintto find out!).masterbranch.If you have any questions about your code changes or any of the points above, please submit your questions along with the pull request and we will try our best to help!
Description of Changes
Added check_imaginary parameter to the function referring to the issue #1145
block is only executed if check_imaginary is True. If set to False, this check is skipped for improved performance when DST changes are not a concern.
Benefits: