Skip to content
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

BUG : Add Deprecation FutureWarning for parse function call in read_excel #51437

Closed
wants to merge 11 commits into from

Conversation

pacificdragon
Copy link

Copy link
Member

@rhshadrach rhshadrach 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 the PR! Some requests below. In order to deprecate passing arguments by position, I think we'll need to change the signature to sheet_name, *args, **kwargs in order tell which arguments are passed by position and which are passed by keyword.

@@ -1564,6 +1566,9 @@ def parse(
"""
Parse specified sheet(s) into a DataFrame.

.. deprecated:: 2.0.0
Arguments other than sheet_name by position may not work.
Copy link
Member

Choose a reason for hiding this comment

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

Specifying arguments other than sheet_name by position is deprecated. Specify arguments by keyword name instead.

pandas/io/excel/_base.py Show resolved Hide resolved
@@ -1572,6 +1577,41 @@ def parse(
DataFrame or dict of DataFrames
DataFrame from the passed in Excel file.
"""
arguments = list(kwds.keys())
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, just iterated over kwds below: for key in kwds

"convert_float",
]
# Check for any invalid kwargs
if [argument for argument in arguments if argument not in allowed_kwargs]:
Copy link
Member

Choose a reason for hiding this comment

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

if any(key in allowed_kwargs for key in kwds):

if [argument for argument in arguments if argument not in allowed_kwargs]:
warnings.warn(
f"{type(self).__name__}.parse is deprecated. "
"Arguments other than sheet_name by position may not work.",
Copy link
Member

Choose a reason for hiding this comment

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

This method is not deprecated, right? Just passing invalid arguments and passing arguments by position.

@simonjayhawkins simonjayhawkins added Bug IO Excel read_excel, to_excel Deprecate Functionality to remove in pandas labels Feb 22, 2023
@pacificdragon
Copy link
Author

@rhshadrach @simonjayhawkins - Closed this by mistake was merging the latest main to my branch. Will resolve mentioned comments and will create a new PR.

@rhshadrach
Copy link
Member

Why not just reopen?

@pacificdragon
Copy link
Author

Why not just reopen?

Okay sorry wasn't aware about it. Reopening now.

@pacificdragon pacificdragon reopened this Feb 27, 2023
@github-actions
Copy link
Contributor

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Mar 30, 2023
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

Sorry this fell off my radar!

Comment on lines +1576 to +1578
if any(key in allowed_kwargs for key in kwds):
warnings.warn(
"Specifying arguments other than sheet_name by position is deprecated."
Copy link
Member

Choose a reason for hiding this comment

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

The if condition here will evaluate to true when a user passes an argument by keyword, no? We only want to warn when an argument is passed by position.

@rhshadrach
Copy link
Member

I'd like to make sure there isn't a better way of deprecating specifying arguments by position as recommended in #51437 (review). cc @mroeschke @phofl

@mroeschke
Copy link
Member

I'd like to make sure there isn't a better way of deprecating specifying arguments by position as recommended in #51437 (review). cc @mroeschke @phofl

I think you can use in deprecate_nonkeyword_arguments in pandas/util/_decorators.py

@mroeschke
Copy link
Member

Thanks for the pull request, but it appears to have gone stale. If interested in continuing, please merge in the main branch, address any review comments and/or failing tests, and we can reopen.

@mroeschke mroeschke closed this Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Deprecate Functionality to remove in pandas IO Excel read_excel, to_excel Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: xl.parse index_col ignoring skiprows
4 participants