-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
FIX: Change limits of power_t param to [0, inf) #31474
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
FIX: Change limits of power_t param to [0, inf) #31474
Conversation
Thanks for creating this Pull Request (and getting involved in open-source!). I think we should handle this via deprecation cycle. This means that code that today uses a negative value will get a warning that in two releases time this will become an invalid value. You can take a look at the section deprecating a parameter of the deprecation guide. We aren't doing exactly the same here, but I think that is the closest example to take inspiration from. In addition to emitting a warning we need to update the docstrings to mention the deprecation and add a test to check that the warning is (not) raised when it should be. |
I have made the changes based on your suggestion. Please let me know if this is what you are looking for and if any further changes need to be made. |
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.
Looks good to me modulo the docstring change and adding a changelog entry
f2ff408
to
796e9f5
Compare
@betatim I have changed the docstring and added the changelog entry, please let me know if this PR can be merged |
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.
LGTM! Thanks for the PR, @ritvi-alagusankar!
796e9f5
to
28086a6
Compare
@virchan gentle ping to help merge the PR |
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.
LGTM. Thanks @ritvi-alagusankar
@ritvi-alagusankar |
Reference Issues/PRs
Fixes #22178. Also see #22215 comment for reference.
What does this implement/fix? Explain your changes.