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

GH10559: Minor improvement: Change read_excel sheet name #16442

Merged
merged 5 commits into from
May 23, 2017
Merged

GH10559: Minor improvement: Change read_excel sheet name #16442

merged 5 commits into from
May 23, 2017

Conversation

abarber4gh
Copy link
Contributor

@abarber4gh abarber4gh commented May 22, 2017

modify io/excel.py and relevant docs (io.rst) to use sheet_name for read_excel
but allow sheetname to still be used for backwards compatibility. add test_excel
to verify that sheet_name and sheetname args produce the same result.

modify io/excel.py and relevant docs (io.rst) to use sheet_name for read_excel
but allow sheetname to still be used for backwards compatibility.  add test_excel
to verify that sheet_name and sheetname args produce the same result.
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.

need to add a note in deprecation section

@@ -200,8 +200,12 @@ def read_excel(io, sheetname=0, header=0, skiprows=None, skip_footer=0,
if not isinstance(io, ExcelFile):
io = ExcelFile(io, engine=engine)

# maintain backwards compatibility by converting sheetname to sheet_name
if 'sheetname' in kwds:
sheet_name = kwds.pop('sheetname')
Copy link
Contributor

Choose a reason for hiding this comment

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

needs a deprecation warning on the original arg. You need to also accept the original arg as a kwarg.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can use pandas.util._decorators.deprecate_kwarg

@@ -544,6 +544,18 @@ def test_date_conversion_overflow(self):
result = self.get_exceldf('testdateoverflow')
tm.assert_frame_equal(result, expected)

# GH10559: Minor improvement: Change to_excel "sheet_name" to "sheetname"
Copy link
Contributor

Choose a reason for hiding this comment

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

comments go inside the function.

def test_sheet_name_and_sheetname(self):
dfref = self.get_csv_refdf('test1')
df1 = self.get_exceldf('test1', sheet_name='Sheet1') # doc
df2 = self.get_exceldf('test1', sheetname='Sheet2') # bkwrds compat
Copy link
Contributor

Choose a reason for hiding this comment

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

this should show a FutureWarning

Copy link
Contributor

@TomAugspurger TomAugspurger May 23, 2017

Choose a reason for hiding this comment

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

You can assert that a warning is issued with thetm.assert_produces_warning context manager.

@jreback jreback added Deprecate Functionality to remove in pandas IO Excel read_excel, to_excel labels May 23, 2017
added @deprecate_kwarg to read_excel as arg changes from sheetname to sheet_name.
moved test comments into function, add assert_produces_warning.
@codecov
Copy link

codecov bot commented May 23, 2017

Codecov Report

Merging #16442 into master will decrease coverage by <.01%.
The diff coverage is 80%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16442      +/-   ##
==========================================
- Coverage   90.42%   90.42%   -0.01%     
==========================================
  Files         161      161              
  Lines       51023    51025       +2     
==========================================
  Hits        46138    46138              
- Misses       4885     4887       +2
Flag Coverage Δ
#multiple 88.26% <80%> (-0.01%) ⬇️
#single 40.17% <40%> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel.py 62.22% <80%> (-0.04%) ⬇️
pandas/core/common.py 91.05% <0%> (-0.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ec31b...5a59cd0. Read the comment docs.

@codecov
Copy link

codecov bot commented May 23, 2017

Codecov Report

Merging #16442 into master will decrease coverage by 2.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #16442      +/-   ##
==========================================
- Coverage   90.42%   88.26%   -2.17%     
==========================================
  Files         161      161              
  Lines       51023    51025       +2     
==========================================
- Hits        46138    45037    -1101     
- Misses       4885     5988    +1103
Flag Coverage Δ
#multiple 88.26% <100%> (ø) ⬆️
#single ?
Impacted Files Coverage Δ
pandas/io/excel.py 62.31% <100%> (+0.05%) ⬆️
pandas/io/sql.py 46.66% <0%> (-48.14%) ⬇️
pandas/core/computation/pytables.py 62.38% <0%> (-29.97%) ⬇️
pandas/io/pytables.py 64.24% <0%> (-28.83%) ⬇️
pandas/io/feather_format.py 72.72% <0%> (-15.16%) ⬇️
pandas/core/computation/common.py 78.57% <0%> (-7.15%) ⬇️
pandas/core/computation/expr.py 84.03% <0%> (-4.46%) ⬇️
pandas/io/clipboard/clipboards.py 22.78% <0%> (-3.8%) ⬇️
pandas/io/formats/console.py 66.66% <0%> (-3.04%) ⬇️
pandas/io/formats/printing.py 87.61% <0%> (-1.77%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49ec31b...4c6da13. Read the comment docs.

Aaron Barber added 2 commits May 22, 2017 21:40
remove manual arg change, use @deprecate_kwarg to read_excel
as arg changes from sheetname to sheet_name.
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.

pls add a note in whatsnew (0.21.0) / Deprecations

@@ -48,7 +48,7 @@
The string could be a URL. Valid URL schemes include http, ftp, s3,
and file. For file URLs, a host is expected. For instance, a local
file could be file://localhost/path/to/workbook.xlsx
sheetname : string, int, mixed list of strings/ints, or None, default 0
sheet_name : string, int, mixed list of strings/ints, or None, default 0

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add sheetname (DEPRECATED) as well

Copy link
Contributor

Choose a reason for hiding this comment

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

There's also the deprecated sphinx directive. I don't see any uses of that, but we can give it a shot here. I think it'd be like

sheetname : string, int, mixed list of strings/ints, or None, default 0

    .. deprecated:: 0.21.0
       Use `sheet_name` instead

sheet_name : string, int, mixed list of strings/ints, or None, default 0

update whats new 0.21.0 Deprecations section noting sheetname deprecated
in favor of sheet_name.
add sheetname deprecation in read_excel() docstring.
@TomAugspurger TomAugspurger merged commit c933098 into pandas-dev:master May 23, 2017
@TomAugspurger
Copy link
Contributor

@abarber4gh thanks, nice job! If you want to check the docs http://pandas-docs.github.io/pandas-docs-travis/generated/pandas.read_excel.html when this build finishes, I'm curious to see how the .. deprecated: directive looks.

@abarber4gh abarber4gh deleted the issue#10559 branch May 23, 2017 20:42
@TomAugspurger TomAugspurger added this to the 0.21.0 milestone May 27, 2017
stangirala pushed a commit to stangirala/pandas that referenced this pull request Jun 11, 2017
* GH10559: Minor improvement: Change to_excel sheet name

modify io/excel.py and relevant docs (io.rst) to use sheet_name for read_excel
but allow sheetname to still be used for backwards compatibility.  add test_excel
to verify that sheet_name and sheetname args produce the same result.

* GH10559: Minor improvement: Change to_excel sheet name

added @deprecate_kwarg to read_excel as arg changes from sheetname to sheet_name.
moved test comments into function, add assert_produces_warning.

* GH10559: Minor improvement: Change to_excel sheet name

remove manual arg change, use @deprecate_kwarg to read_excel
as arg changes from sheetname to sheet_name.

* GH10559: Minor improvement: Change to_excel sheet name

shorten lines under 79 char.

* GH10559: Minor improvement: Change to_excel sheet name

update whats new 0.21.0 Deprecations section noting sheetname deprecated
in favor of sheet_name.
add sheetname deprecation in read_excel() docstring.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Minor improvement: Change to_excel "sheet_name" to "sheetname"
3 participants