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.

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

DEPR: Deprecate parse_cols in read_excel #17774

Merged
merged 2 commits into from
Oct 4, 2017
Merged

DEPR: Deprecate parse_cols in read_excel #17774

merged 2 commits into from
Oct 4, 2017

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Oct 4, 2017 •
edited by jreback
Loading

Will now use "usecols" just like in read_csv.
closes #4988.

@gfyoung
DEPR: Deprecate parse_cols in read_excel
517b726 
Will now use "usecols" just like in read_csv.

xref pandas-devgh-4988.
@gfyoung gfyoung added Deprecate Functionality to remove in pandas IO Excel read_excel, to_excel labels Oct 4, 2017
@gfyoung gfyoung added this to the 0.21.0 milestone Oct 4, 2017
@codecov
Copy link

codecov bot commented Oct 4, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17774      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49872    49873       +1     
==========================================
- Hits        45514    45506       -8     
- Misses       4358     4367       +9
Flag Coverage Δ
#multiple 89.04% <100%> (ø) ⬆️
#single 40.24% <100%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel.py 80.4% <100%> (+0.02%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️

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 81694dc...517b726. Read the comment docs.

@codecov
Copy link

codecov bot commented Oct 4, 2017 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17774      +/-   ##
==========================================
- Coverage   91.26%   91.24%   -0.02%     
==========================================
  Files         163      163              
  Lines       49872    49873       +1     
==========================================
- Hits        45514    45506       -8     
- Misses       4358     4367       +9
Flag Coverage Δ
#multiple 89.04% <100%> (ø) ⬆️
#single 40.24% <27.27%> (-0.06%) ⬇️
Impacted Files Coverage Δ
pandas/io/excel.py 80.4% <100%> (+0.02%) ⬆️
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/frame.py 97.73% <0%> (-0.1%) ⬇️

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 81694dc...62d6cac. Read the comment docs.

jreback
jreback requested changes Oct 4, 2017
View reviewed changes
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.

all looks good. I would internally change the kwarg though.

pandas/io/excel.py
@@ -226,7 +231,7 @@ def read_excel(io, sheet_name=0, header=0, skiprows=None, skip_footer=0,

return io._parse_excel(
sheetname=sheet_name, header=header, skiprows=skiprows, names=names,
index_col=index_col, parse_cols=parse_cols, parse_dates=parse_dates,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change the internal kwargs all the way down to usecols

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good point. Will do.

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW, this PR is just addressing #4988, not closing it?

Copy link
Contributor

Choose a reason for hiding this comment

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

how so?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well because the conversation arose from #4988, so at the very least we're addressing it. However, I wasn't sure if we were closing the issue after this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really remember this, can you give me a 1-liner what the issue is? (I thought it was just a rename / deprecation)

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue was regarding "inconsistent naming between read_excel and read_csv" That was really it. I didn't see anything naming inconsistencies besides this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok to close that issue (I changed at top)

@jsexauer jsexauer mentioned this pull request Oct 4, 2017
Closed
1 task
@gfyoung
Copy link
Member Author

gfyoung commented Oct 4, 2017

@jreback : Besides deciding whether to leave #4988 open after merging, it's all green. PTAL.

@jreback
Copy link
Contributor

jreback commented Oct 4, 2017

looks fine. just run the excel tests locally and see if you are getting any deprecation warnings (and then suppress or better yet change). also on the docs you may need to change some older code to a code-block (and may need to fix text in the docs in any event).

@gfyoung
Copy link
Member Author

gfyoung commented Oct 4, 2017

@jreback : I replaced all instances of parse_cols in the io.rst doc. Also, I'm getting no warnings when running the tests. GitHub search seems to indicate that that is all.

If all is good, can merge this in.

@jreback jreback merged commit 6b29fc0 into pandas-dev:master Oct 4, 2017
@jreback
Copy link
Contributor

jreback commented Oct 4, 2017

thanks @gfyoung !

@gfyoung gfyoung deleted the parse-cols-depr-excel branch October 5, 2017 03:37
ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 16, 2017
@gfyoung
DEPR: Deprecate parse_cols in read_excel (pandas-dev#17774)
c1e68cf 
closes pandas-dev#4988
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
@gfyoung @alanbato
DEPR: Deprecate parse_cols in read_excel (pandas-dev#17774)
1da2b76 
closes pandas-dev#4988
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@gfyoung @No-Stream
DEPR: Deprecate parse_cols in read_excel (pandas-dev#17774)
f8c385b 
closes pandas-dev#4988
This was referenced Mar 25, 2018
Closed
Closed
@jreback jreback mentioned this pull request Jun 30, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Deprecate Functionality to remove in pandas IO Excel read_excel, to_excel
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

ENH: inconsistent naming convention for read_csv and read_excel column selection
2 participants
@gfyoung @jreback