Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

ENH: Implement PDEP-17 #61468

wants to merge 9 commits into from
+464 −166

Conversation

rhshadrach
Copy link
Member

@rhshadrach rhshadrach commented May 20, 2025 •
edited
Loading

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest 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

rhshadrach
rhshadrach commented May 20, 2025
View reviewed changes
pandas/errors/__init__.py Outdated
Comment on lines 94 to 97
class PandasChangeWarning(Warning):
"""
Warning raised for any an upcoming change.
"""
Copy link
Member Author

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!

Copy link
Member Author

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.

@rhshadrach rhshadrach added this to the 3.0 milestone May 20, 2025
@rhshadrach rhshadrach added Enhancement Warnings Warnings that appear or should be added to pandas PDEP pandas enhancement proposal labels May 20, 2025
@datapythonista datapythonista removed the PDEP pandas enhancement proposal label May 20, 2025
@datapythonista
Copy link
Member

Seems like the label PDEP can only be used for the PDEP themselves. Otherwise building the website fails, as it expects the PR to be a PDEP that needs to be rendered in the roadmap.

Dr-Irv
Dr-Irv requested changes May 21, 2025
View reviewed changes
Copy link
Contributor

@Dr-Irv Dr-Irv left a 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.

@rhshadrach
Copy link
Member Author

@Dr-Irv - Thanks for the review! I'm negative on the name PandasWarning for a class that has to do with deprecations. pandas emits many warnings, not all are about deprecations.

@Dr-Irv
Copy link
Contributor

Dr-Irv commented May 21, 2025

@Dr-Irv - Thanks for the review! I'm negative on the name PandasWarning for a class that has to do with deprecations. pandas emits many warnings, not all are about deprecations.

Maybe a different name would solve that? Maybe PandasDeprecationOrFutureWarning or something like that.

@rhshadrach
Copy link
Member Author

Maybe a different name would solve that? Maybe PandasDeprecationOrFutureWarning or something like that.

Currently using PandasChangeWarning, which has my personal preference so far. Always open to other options or opinions!

Aloqeely
Aloqeely reviewed May 30, 2025
View reviewed changes
Copy link
Member

@Aloqeely Aloqeely left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Warning raised for any an upcoming change.
Warning raised for any upcoming change.

pandas/errors/__init__.py Outdated
Comment on lines 157 to 160

See Also
--------
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
See Also
--------
errors.Pandas4Warning : Class for deprecations to be enforced in pandas 4.0.

pandas/tests/util/test_deprecate_nonkeyword_arguments.py Outdated
@@ -4,13 +4,16 @@

import inspect

from pandas.errors import Pandas4Warning
Copy link
Member

@Aloqeely Aloqeely May 30, 2025 •
edited
Loading

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@rhshadrach
Copy link
Member Author

I think there should be separate docs about the deprecation policy and these classes, aside from what is in the whatsnew document.

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.

Dr-Irv
Dr-Irv requested changes Jun 3, 2025
View reviewed changes
Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@rhshadrach rhshadrach requested a review from Dr-Irv June 4, 2025 00:42
@rhshadrach
Copy link
Member Author

@bashtage - pandas_datareader is using the deprecate_kwarg decorator. I'm modifying it here, is this an issue?

Dr-Irv
Dr-Irv approved these changes Jun 4, 2025
View reviewed changes
Copy link
Contributor

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm good with the current version. I'll let others decide when to merge.

@rhshadrach rhshadrach requested review from Aloqeely and mroeschke June 4, 2025 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dr-Irv Dr-Irv Dr-Irv approved these changes

@Aloqeely Aloqeely Awaiting requested review from Aloqeely

@mroeschke mroeschke Awaiting requested review from mroeschke

Assignees
No one assigned
Labels
Enhancement Warnings Warnings that appear or should be added to pandas
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@rhshadrach @datapythonista @Dr-Irv @Aloqeely