-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DEPR: Add deprecation warning for factorize() order keyword #19751
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
Changes from 1 commit
261044a
ab47916
616c357
18eb031
6a34d9e
4c83659
1b10ca2
b5daae0
bd9717e
b7ed3c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,9 +34,14 @@ def _f2(new=False): | |
def _f3(new=0): | ||
return new | ||
|
||
@deprecate_kwarg('old', None) | ||
def _f4(old=True, unchanged=True): | ||
return old | ||
|
||
self.f1 = _f1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are u adding all this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In a nutshell, to test that warnings are only raised if I gave
With f4 I can test that:
Hope that makes sense. |
||
self.f2 = _f2 | ||
self.f3 = _f3 | ||
self.f4 = _f4 | ||
|
||
def test_deprecate_kwarg(self): | ||
x = 78 | ||
|
@@ -72,6 +77,15 @@ def test_bad_deprecate_kwarg(self): | |
def f4(new=None): | ||
pass | ||
|
||
def test_deprecate_keyword(self): | ||
x = 9 | ||
with tm.assert_produces_warning(FutureWarning): | ||
result = self.f4(old=x) | ||
assert result is x | ||
with tm.assert_produces_warning(None): | ||
result = self.f4(unchanged=x) | ||
assert result is True | ||
|
||
|
||
def test_rands(): | ||
r = tm.rands(10) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,8 +65,9 @@ def deprecate_kwarg(old_arg_name, new_arg_name, mapping=None, stacklevel=2): | |
---------- | ||
old_arg_name : str | ||
Name of argument in function to deprecate | ||
new_arg_name : str | ||
Name of preferred argument in function | ||
new_arg_name : str | None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually this is written as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching this -- didn't realize this is the case. Please refer to 4c83659 |
||
Name of preferred argument in function. Use none to raise warning that | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. none -> None There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for catching that. Please refer to ab47916 |
||
``old_arg_name`` keyword is deprecated. | ||
mapping : dict or callable | ||
If mapping is present, use it to translate old arguments to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add a couple of examples to the doc-string here? e.g. doing a rename and removing a kwarg There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a couple examples for removing a keyword. I did see a couple good examples of how to raise a warning when a keyword will be deprecated for another: Do let me know if you think we should still add some more examples for renaming kwargs. |
||
new arguments. A callable must do its own value checking; | ||
|
@@ -107,6 +108,16 @@ def _deprecate_kwarg(func): | |
@wraps(func) | ||
def wrapper(*args, **kwargs): | ||
old_arg_value = kwargs.pop(old_arg_name, None) | ||
|
||
if new_arg_name is None and old_arg_value is not None: | ||
msg = ( | ||
"the '{old_name}' keyword is no longer supported" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the {old_name} keyword is deprecated and will be removed in a future version is our typical phrasing here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. Please refer to ab47916 |
||
"please takes steps to stop use of '{old_name}'" | ||
).format(old_name=old_arg_name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since we have this new capability for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. I'll take a look around and open a separate PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jreback following up. I found that the remaining 'manual' deprecation warnings in the repo target an entire function or class (and in some cases class attributes). I didn't find any for keyword specific deprecations. I suppose I can create an analogous decorator called |
||
warnings.warn(msg, FutureWarning, stacklevel=stacklevel) | ||
kwargs[old_arg_name] = old_arg_value | ||
return func(*args, **kwargs) | ||
|
||
if old_arg_value is not None: | ||
if mapping is not None: | ||
if hasattr(mapping, 'get'): | ||
|
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.
ca you add a test which catches this, AND see if we have any existing cases of actually passing this arg
Uh oh!
There was an error while loading. Please reload this page.
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.
Sure thing. I added two additional tests. Please refer to ab47916.
I did not find any cases in the Repo where
factorize()
is called with anorder
parameter.