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 from_csv in favor of read_csv #17812

Merged
merged 1 commit into from
Oct 10, 2017
Merged

DEPR: Deprecate from_csv in favor of read_csv #17812

merged 1 commit into from
Oct 10, 2017

Conversation

gfyoung
Copy link
Member

@gfyoung gfyoung commented Oct 7, 2017

Title is self-explanatory. Closes #4191.

@gfyoung gfyoung added Deprecate Functionality to remove in pandas IO CSV read_csv, to_csv labels Oct 7, 2017
@gfyoung gfyoung added this to the 0.21.0 milestone Oct 7, 2017
@codecov
Copy link

codecov bot commented Oct 7, 2017 •
edited

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #17812      +/-   ##
==========================================
- Coverage   91.24%   91.22%   -0.02%     
==========================================
  Files         163      163              
  Lines       49994    49995       +1     
==========================================
- Hits        45615    45607       -8     
- Misses       4379     4388       +9
Flag Coverage Δ
#multiple 89.02% <100%> (ø) ⬆️
#single 40.24% <0%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/core/series.py 94.89% <ø> (ø) ⬆️
pandas/core/frame.py 97.74% <100%> (-0.1%) ⬇️
pandas/io/gbq.py 25% <0%> (-58.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 3ba2cff...28f8097. Read the comment docs.

auvipy
auvipy approved these changes Oct 7, 2017
View reviewed changes
Copy link

@auvipy auvipy left a comment

Choose a reason for hiding this comment

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

looks ok

jreback
jreback requested changes Oct 7, 2017
View reviewed changes
pandas/tests/frame/test_to_csv.py Outdated
assert_frame_equal(np.isinf(self.frame),
np.isinf(recons), check_names=False)
with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Contributor

@jreback jreback Oct 7, 2017 •
edited by gfyoung

Choose a reason for hiding this comment

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

i would fix almost all of these to just call read_csv (maybe leave a couple which catch the warning)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

also any test that is not for to_csv could be moved to the parsers testing
prob are duplicates (of existing tests)

can be done in a follow up if u want (make an issue in that case)

Copy link
Member Author

Choose a reason for hiding this comment

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

These are all for to_csv. They are round-trip checks mostly.

pandas/tests/series/test_io.py Outdated
check_stacklevel=False):
self.ts.to_csv(path)
ts = Series.from_csv(path)
assert_series_equal(self.ts, ts, check_names=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

pandas/tests/series/test_io.py Outdated

assert_series_equal(s, s2)
with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@gfyoung
Copy link
Member Author

gfyoung commented Oct 7, 2017

@jreback : I took out all but a few instances of from_csv for testing purposes, and all is green. PTAL.

@jreback
Copy link
Contributor

jreback commented Oct 8, 2017

need a whatsnew note

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

gfyoung commented Oct 8, 2017

need a whatsnew note

Oops. Added.

@gfyoung
Copy link
Member Author

gfyoung commented Oct 8, 2017

@jreback : Added whatsnew. PTAL.

jorisvandenbossche
jorisvandenbossche reviewed Oct 9, 2017
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

minor comment, looks good!

pandas/tests/frame/test_to_csv.py Outdated
assert_frame_equal(self.tsframe, recons)

with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Member

Choose a reason for hiding this comment

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

the check_stacklevel=False should not be needed

Copy link
Member Author

Choose a reason for hiding this comment

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

True, fixed.

jreback
jreback reviewed Oct 9, 2017
View reviewed changes
pandas/tests/frame/test_to_csv.py Outdated
with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
depr_recons = DataFrame.from_csv(path)
assert_frame_equal(self.tsframe, depr_recons)
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe move this round-trip out to a separate test function (just the to_csv/from_csv), easier to read (it will be obvious then why you defined self.read_csv then and easier to remove later

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

jreback
jreback reviewed Oct 9, 2017
View reviewed changes
pandas/tests/series/test_io.py
assert ts.name is None
assert ts.index.name is None

# GH10483
with tm.assert_produces_warning(FutureWarning,
check_stacklevel=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jreback
Copy link
Contributor

jreback commented Oct 9, 2017

pls add from_csv to the class level _deprecations parameter

@gfyoung
Copy link
Member Author

gfyoung commented Oct 10, 2017

pls add from_csv to the class level _deprecations parameter

Yep, done as well.

@jreback
Copy link
Contributor

jreback commented Oct 10, 2017

lgtm. merge on green.

@gfyoung gfyoung merged commit 0f548d4 into pandas-dev:master Oct 10, 2017
@gfyoung gfyoung deleted the from-csv-depr branch October 10, 2017 04:52
ghost pushed a commit to reef-technologies/pandas that referenced this pull request Oct 16, 2017
@gfyoung
DEPR: Deprecate from_csv in favor of read_csv (pandas-dev#17812)
ab29752 
Closes pandas-devgh-4191.
alanbato pushed a commit to alanbato/pandas that referenced this pull request Nov 10, 2017
@gfyoung @alanbato
DEPR: Deprecate from_csv in favor of read_csv (pandas-dev#17812)
e56e8ae 
Closes pandas-devgh-4191.
@alefnula alefnula mentioned this pull request Nov 26, 2017
Closed
4 tasks
No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@gfyoung @No-Stream
DEPR: Deprecate from_csv in favor of read_csv (pandas-dev#17812)
044106c 
Closes pandas-devgh-4191.
@jreback jreback mentioned this pull request Jun 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@jreback jreback jreback approved these changes

@auvipy auvipy auvipy approved these changes

Assignees
No one assigned
Labels
Deprecate Functionality to remove in pandas IO CSV read_csv, to_csv
Projects
None yet
Milestone
0.21.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gfyoung @jreback @jorisvandenbossche @auvipy