BUG: Fix line_terminator option #23608

BenRussert · 2018-11-09T23:30:46Z

tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff

pep8speaks · 2018-11-09T23:30:47Z

Hello @BenRussert! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on January 16, 2019 at 06:09 Hours UTC

BenRussert · 2018-11-09T23:36:08Z

This line won't ever reach 'os.linesep' if the default argument is '\n'. I noticed the issue because windows to_csv, and to_clipboard write \n when the docs indicate the default windows \r\n

I've only tried the tests locally on ubuntu, but I wonder if this will break tests for people running on windows if \n is hardcoded in the test. (I'll look later if this makes sense)

codecov · 2018-11-10T00:19:34Z

Codecov Report

Merging #23608 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #23608   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52358    52358           
=======================================
  Hits        48373    48373           
  Misses       3985     3985

Flag	Coverage Δ
#multiple	`90.81% <ø> (ø)`	⬆️
#single	`43.07% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/io/formats/csvs.py	`98.22% <ø> (ø)`	⬆️

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 33f91d8...9897a49. Read the comment docs.

gfyoung · 2018-11-11T01:03:45Z

@BenRussert : Good start! Definitely going to need some tests.

jreback · 2018-12-03T01:50:56Z

@BenRussert can you add some tests for this

BenRussert · 2018-12-09T21:38:25Z

I've added tests @gfyoung and @jreback, sorry for the delay!

I also found an issue! This closes #11720

pandas/tests/io/formats/test_to_csv.py

WillAyd

Some minor nits on my end. FYI if you merge master going forward you shouldn't need to force push changes which makes tracking easier

pandas/tests/io/formats/test_to_csv.py

BenRussert · 2018-12-14T03:18:15Z

See also #20353, #11720. to_csv issue was fixed in #21406, but the issue remains for windows users in to_clipboard.

… platforms

jreback · 2018-12-14T13:52:30Z

pandas/io/formats/csvs.py

@@ -30,7 +30,7 @@ class CSVFormatter(object):
    def __init__(self, obj, path_or_buf=None, sep=",", na_rep='',
                 float_format=None, cols=None, header=True, index=True,
                 index_label=None, mode='w', nanRep=None, encoding=None,
-                 compression='infer', quoting=None, line_terminator='\n',
+                 compression='infer', quoting=None, line_terminator=None,


this is going to need a doc-string versionchanged, and an update

On closer look, #24290 covered this. In fact, the same PR may have corrected the entire issue as long as the CSVFormatter class is always called with line_terminator=None.

I made a minor docs update to reflect that PR, let me know if you had something else in mind.

I think the reference issue is off? in any event, your change looks fine.

jreback · 2018-12-14T13:52:50Z

is there an associated issue?

BenRussert · 2018-12-15T03:00:41Z

is there an associated issue?

#11720 describes the behavior (which may have been fixed by #24290). There's also at least one or two other issues out there that can be closed.

jreback · 2019-01-05T15:38:48Z

can you merge master

BenRussert · 2019-01-12T17:34:36Z

can you merge master

@jreback DId you want me to push a merge commit? Or rebase and force?

jreback · 2019-01-12T17:44:52Z

the former

jreback · 2019-01-13T19:34:48Z

pandas/tests/io/formats/test_to_csv.py

@@ -561,3 +561,20 @@ def test_to_csv_compression(self, compression_only,
            result = pd.read_csv(path, index_col=0,
                                 compression=read_compression)
            tm.assert_frame_equal(result, df)
+
+
+def test_csv_formatter_line_terminator_default(monkeypatch):


rather than this, can you just modify the appropriate tests to pass in ['\n', os.linesep, None] as parameters (we might already have this)

modify the appropriate tests to pass

@jreback There isn't an existing test of the CSVFormatter directly.

The intent of the PR is to ensure that if no line_terminator is specified, the default is os.linesep. Currently, CSVFormatter must be called with line_terminator=None or else \n will be used.

When I opened this PR, I was looking at v0.24.3, so I didn't realize the bug had been fixed in #21406 by calling the CSVFormatter class with line_terminator=None. I thought I might as well continue the PR so future devs don't have to remember to call CSVFormatter(line_terminator=None) if they expect the platform specific os.linesep behavior. Since most devs use \n platforms, it would be easy to miss this.

i don't think this test is necessary and slightly confusing, can you remove.

jreback · 2019-01-16T02:15:34Z

pandas/io/formats/csvs.py

@@ -30,7 +30,7 @@ class CSVFormatter(object):
    def __init__(self, obj, path_or_buf=None, sep=",", na_rep='',
                 float_format=None, cols=None, header=True, index=True,
                 index_label=None, mode='w', nanRep=None, encoding=None,
-                 compression='infer', quoting=None, line_terminator='\n',
+                 compression='infer', quoting=None, line_terminator=None,


I think the reference issue is off? in any event, your change looks fine.

jreback · 2019-01-16T02:16:09Z

pandas/tests/io/formats/test_to_csv.py

@@ -561,3 +561,20 @@ def test_to_csv_compression(self, compression_only,
            result = pd.read_csv(path, index_col=0,
                                 compression=read_compression)
            tm.assert_frame_equal(result, df)
+
+
+def test_csv_formatter_line_terminator_default(monkeypatch):


i don't think this test is necessary and slightly confusing, can you remove.

WillAyd · 2019-01-16T17:32:01Z

@BenRussert can you update your OP to reflect which issue this addresses? Somewhat confused reading through the comments as to what problem this solves

BenRussert · 2019-01-29T22:57:46Z

I'll just close and say that #24290 fixed what I was going after. The default argument vs calling with None is what confused me.

gfyoung added Output-Formatting __repr__ of pandas objects, to_string IO CSV read_csv, to_csv labels Nov 11, 2018

BenRussert commented Dec 10, 2018

View reviewed changes

pandas/tests/io/formats/test_to_csv.py Outdated Show resolved Hide resolved

BenRussert added 2 commits December 10, 2018 22:20

TST: Add test for default line_terminator argument

1468d25

BUG: Fix line_terminator option

f3deb4e

BenRussert force-pushed the line-terminators branch from b192086 to f3deb4e Compare December 11, 2018 05:31

WillAyd requested changes Dec 11, 2018

View reviewed changes

pandas/tests/io/formats/test_to_csv.py Outdated Show resolved Hide resolved

pandas/tests/io/formats/test_to_csv.py Outdated Show resolved Hide resolved

pandas/tests/io/formats/test_to_csv.py Outdated Show resolved Hide resolved

BenRussert and others added 2 commits December 10, 2018 23:34

CLN: Whitespace

a1d91a9

CLN: review fixes

8cf3b69

WillAyd reviewed Dec 13, 2018

View reviewed changes

pandas/tests/io/formats/test_to_csv.py Outdated Show resolved Hide resolved

TST: Use foo/bar line terminator placeholders for consistentcy across…

456ec1b

… platforms

jreback requested changes Dec 14, 2018

View reviewed changes

DOC: Update io docs for pandas-dev#21406

f562d82

Merge 'upstream/master'

a0ba3b3

jreback requested changes Jan 13, 2019

View reviewed changes

jreback requested changes Jan 16, 2019

View reviewed changes

Remove unecessary test

9897a49

BenRussert closed this Jan 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: Fix line_terminator option #23608

BUG: Fix line_terminator option #23608

BenRussert commented Nov 9, 2018 •

edited

Loading

pep8speaks commented Nov 9, 2018 •

edited

Loading

BenRussert commented Nov 9, 2018

codecov bot commented Nov 10, 2018 •

edited

Loading

gfyoung commented Nov 11, 2018 •

edited

Loading

jreback commented Dec 3, 2018

BenRussert commented Dec 9, 2018

WillAyd left a comment

BenRussert commented Dec 14, 2018

jreback Dec 14, 2018

BenRussert Dec 15, 2018

jreback Jan 16, 2019

jreback commented Dec 14, 2018

BenRussert commented Dec 15, 2018

jreback commented Jan 5, 2019

BenRussert commented Jan 12, 2019

jreback commented Jan 12, 2019

jreback Jan 13, 2019

BenRussert Jan 14, 2019

jreback Jan 16, 2019

jreback Jan 16, 2019

jreback Jan 16, 2019

WillAyd commented Jan 16, 2019

BenRussert commented Jan 29, 2019

BUG: Fix line_terminator option #23608

BUG: Fix line_terminator option #23608

Conversation

BenRussert commented Nov 9, 2018 • edited Loading

pep8speaks commented Nov 9, 2018 • edited Loading

Comment last updated on January 16, 2019 at 06:09 Hours UTC

BenRussert commented Nov 9, 2018

codecov bot commented Nov 10, 2018 • edited Loading

Codecov Report

gfyoung commented Nov 11, 2018 • edited Loading

jreback commented Dec 3, 2018

BenRussert commented Dec 9, 2018

WillAyd left a comment

Choose a reason for hiding this comment

BenRussert commented Dec 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback commented Dec 14, 2018

BenRussert commented Dec 15, 2018

jreback commented Jan 5, 2019

BenRussert commented Jan 12, 2019

jreback commented Jan 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillAyd commented Jan 16, 2019

BenRussert commented Jan 29, 2019

BenRussert commented Nov 9, 2018 •

edited

Loading

pep8speaks commented Nov 9, 2018 •

edited

Loading

codecov bot commented Nov 10, 2018 •

edited

Loading

gfyoung commented Nov 11, 2018 •

edited

Loading