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

DEPR: deprecate non keyword arguments in read_excel #34418

Merged

Conversation

topper-123
Copy link
Contributor

@topper-123 topper-123 commented May 27, 2020

Follow-up to #27573.

Allows two non-keyword arguments, io and sheet_name. I think sheet_name is quite often (e.g. Interactively) supplied without being a keyword argument and requiring it will just be needlessly annoying.

Also some clean-up in pandas/tests/io/excel.

@topper-123 topper-123 changed the title DEPR: deprecate non keyword arguments to read_excel DEPR: deprecate non keyword arguments in read_excel May 27, 2020
@topper-123 topper-123 marked this pull request as ready for review May 28, 2020 00:27
@@ -33,7 +33,7 @@ def test_read_writer_table():
columns=["Column 1", "Unnamed: 2", "Column 3"],
)

result = pd.read_excel("writertable.odt", "Table1", index_col=0)
result = pd.read_excel("writertable.odt", sheet_name="Table1", index_col=0)
Copy link
Member

Choose a reason for hiding this comment

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

Is sheet_name required to be passed like this or still OK positionally? I assume the latter from allowed_args being 2 but maybe am misreading

+/- 0 on making this required as a keyword argument. It would seem a rather natural positional argument so maybe not worth the churn, but not a strong opinion

Copy link
Contributor Author

@topper-123 topper-123 May 28, 2020

Choose a reason for hiding this comment

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

Is sheet_name required to be passed like this or still OK positionally?

This is just a cleanup, I think it's more readable with keyword arguments.

I think exel files are very well known, so people will easily infer that the second parameter is the sheet name, and making positional argument for sheet_name fail, e.g. in jupyter notebook could maybe be seen as too restrictive. This is not a strong opinion, so if making it a required kwarg is seen as better, I can do that.

@topper-123 topper-123 added IO Excel read_excel, to_excel API - Consistency Internal Consistency of API/Behavior labels May 28, 2020
@topper-123 topper-123 added this to the 1.1 milestone May 28, 2020
@jreback jreback added the Deprecate Functionality to remove in pandas label May 28, 2020
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

lgtm. can you add this issue to the deprecation removal 2.0 issue; @WillAyd over to you.

@WillAyd WillAyd merged commit 043b609 into pandas-dev:master May 29, 2020
@WillAyd
Copy link
Member

WillAyd commented May 29, 2020

Great PR @topper-123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API - Consistency Internal Consistency of API/Behavior Deprecate Functionality to remove in pandas IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants