-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
ENH: Implement PDEP-17 #61468
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
base: main
Are you sure you want to change the base?
ENH: Implement PDEP-17 #61468
Conversation
pandas/errors/__init__.py
Outdated
class PandasChangeWarning(Warning): | ||
""" | ||
Warning raised for any an upcoming change. | ||
""" |
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.
Not in love with this name, but PandasDeprecationWarning
is taken below. Bikeshedding here is most welcome!
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.
We could also make this internal if we want to not have it be public.
Seems like the label |
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.
I think there should be separate docs about the deprecation policy and these classes, aside from what is in the whatsnew document.
@Dr-Irv - Thanks for the review! I'm negative on the name |
Maybe a different name would solve that? Maybe |
Currently using |
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.
Thanks for working on this!
pandas/errors/__init__.py
Outdated
@@ -91,6 +91,96 @@ class PerformanceWarning(Warning): | |||
""" | |||
|
|||
|
|||
class PandasChangeWarning(Warning): | |||
""" | |||
Warning raised for any an upcoming change. |
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.
Warning raised for any an upcoming change. | |
Warning raised for any upcoming change. |
pandas/errors/__init__.py
Outdated
|
||
See Also | ||
-------- | ||
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
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.
See Also | |
-------- | |
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
@@ -4,13 +4,16 @@ | |||
|
|||
import inspect | |||
|
|||
from pandas.errors import Pandas4Warning |
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.
Will this test case need to be updated every version? I think having a variable that points to the current version's warning might reduce some of the work.
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.
Then that variable will just have to be updated instead, seems about the same amount of work to me.
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.
I suppose if this is needed in multiple files, then it could become more of a hassle. Will add.
I've added some implementation details to PDEP-17, and more details to the whatsnew. If you'd like to see this documented somewhere else, can you detail where. |
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.
Looking good!
pandas/errors/__init__.py
Outdated
|
||
See Also | ||
-------- | ||
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
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.
Shouldn't this be errors.PandasDeprecationWarning
? i.e., the see also should refer to the general non-version specific class
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.
Absolutely no objection to changing it, but I'm curious as to the reason. I added the See Also
sections only because our code-checks fail without it.
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.
Because otherwise we'd have to change the docs with each new release.
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.
Thanks - I added line items here for all classes that aren't versioned (and the class itself).
pandas/errors/__init__.py
Outdated
|
||
See Also | ||
-------- | ||
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
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.
same comment as above
pandas/errors/__init__.py
Outdated
|
||
See Also | ||
-------- | ||
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
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.
ditto
pandas/errors/__init__.py
Outdated
See Also | ||
-------- | ||
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0. |
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.
maybe delete this? Otherwise we have to update this docs upon every new major release.
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.
Change it to a PandasDeprecationWarning instead? Otherwise we'll have to add an exception to the code-checks to allow a docstring without any See Also.
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.
Cross referencing to PandasDeprecationWarning
makes sense, and then PandasDeprecationWarning
can cross-reference to PandasFutureWarning
def f1(): | ||
warnings.warn("f1", PandasChangeWarning) | ||
|
||
|
||
def test_function_warns_pandas_deprecation_warning(): | ||
with tm.assert_produces_warning(PandasChangeWarning): | ||
f1() | ||
|
||
|
||
@deprecate_kwarg(PandasChangeWarning, "old", new_arg_name="new") | ||
def f2(new=0): | ||
return new | ||
|
||
|
||
def test_decorator_warns_pandas_deprecation_warning(): | ||
with tm.assert_produces_warning(PandasChangeWarning): | ||
f2(old=1) |
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.
Shouldn't you add tests for PandasDeprecationWarning
, PandasFutureWarning
and PandasPendingDeprecationWarning
?
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.
I deleted this file and added one more test to tests/test_errors.py
.
@bashtage - pandas_datareader is using the |
doc/source/whatsnew/vX.X.X.rst
file if fixing a bug or adding a new feature.Continuation of #58169
Still needs tests and some cleanup with the docs and decorator arguments, but I'd like to get a first look.
cc @Dr-Irv @Aloqeely