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

CLN: Remove ExcelWriter.sheetname #26464

Merged
merged 10 commits into from
May 25, 2019

Conversation

mroeschke
Copy link
Member

cc @WillAyd

@mroeschke mroeschke added Clean IO Excel read_excel, to_excel labels May 20, 2019
Copy link
Member

@WillAyd WillAyd 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! Does this raise now? IIRC we have had some issues with confusion in the past where the kwargs would silently get swallowed (see #25723 as an example with the ExcelWriter) so this may have an unfortunate side effect of working but in a way that silently does the wrong thing.

I see there's already a special case to raise a ValueError is sheet is supplied so might be a good idea to do the same thing here if this doesn't raise

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #26464 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26464      +/-   ##
==========================================
- Coverage   91.76%   91.74%   -0.02%     
==========================================
  Files         174      174              
  Lines       50673    50665       -8     
==========================================
- Hits        46498    46485      -13     
- Misses       4175     4180       +5
Flag Coverage Δ
#multiple 90.26% <100%> (-0.01%) ⬇️
#single 41.71% <0%> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel/_base.py 91.85% <100%> (-0.29%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.6% <0%> (-0.11%) ⬇️

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 374478c...1b1c7a3. Read the comment docs.

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #26464 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #26464      +/-   ##
==========================================
- Coverage   91.74%   91.74%   -0.01%     
==========================================
  Files         174      174              
  Lines       50763    50755       -8     
==========================================
- Hits        46575    46563      -12     
- Misses       4188     4192       +4
Flag Coverage Δ
#multiple 90.25% <ø> (-0.01%) ⬇️
#single 41.71% <ø> (-0.08%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel/_base.py 91.85% <ø> (-0.29%) ⬇️
pandas/io/gbq.py 78.94% <0%> (-10.53%) ⬇️
pandas/core/frame.py 97.02% <0%> (-0.12%) ⬇️

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 44d5498...569a649. Read the comment docs.

@mroeschke
Copy link
Member Author

You were right, @WillAyd. sheetname would just get swallowed by **kwargs and not raise. I added additional code & test for the correct raising behavior.

@@ -269,7 +269,7 @@ Deprecations
Removal of prior version deprecations/changes
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- Removed ``Panel`` (:issue:`25047`, :issue:`25191`, :issue:`25231`)
-
- Removed the previously deprecated ``ExcelWriter.sheetname`` (:issue:`16442`, :issue:`20938`)
Copy link
Member

Choose a reason for hiding this comment

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

Actually is this part of ExcelWriter or read_excel? From the diff I think this should be changed to the latter

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch! I shamelessly copied ExcelWriter from the deprecated issue (which was probably incorrect)

@WillAyd WillAyd added this to the 0.25.0 milestone May 20, 2019
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

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

lgtm

@jreback
Copy link
Contributor

jreback commented May 24, 2019

lgtm. can you merge master (as @WillAyd test refactor was merged); merge on green.

@jreback
Copy link
Contributor

jreback commented May 24, 2019

tick the box in #13777 when merged.

Copy link
Member

@gfyoung gfyoung left a comment

Choose a reason for hiding this comment

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

LGTM

Leave it to you if you want to throw parse_cols in here as well

@gfyoung gfyoung merged commit ba48fc4 into pandas-dev:master May 25, 2019
@gfyoung
Copy link
Member

gfyoung commented May 25, 2019

Thanks @mroeschke !

@mroeschke mroeschke deleted the remove_excel_sheetname branch May 25, 2019 15:51
another-green pushed a commit to another-green/pandas that referenced this pull request May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean IO Excel read_excel, to_excel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants