-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
Fix DataFrame.to_string() justification (2) #22505
Conversation
|
||
for df, expected in zip(dfs, exs): | ||
df_s = df.to_string(index=False) | ||
assert df_s == expected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely should use pytest.mark.parametrize
for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the code style to match other tests in the same file.
Codecov Report
@@ Coverage Diff @@
## master #22505 +/- ##
==========================================
- Coverage 92.18% 92.03% -0.16%
==========================================
Files 169 169
Lines 50820 50778 -42
==========================================
- Hits 46850 46735 -115
- Misses 3970 4043 +73
Continue to review full report at Codecov.
|
lgtm, but I think the changelog needs to be moved to @jreback can you take a look and see if you're happy with this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change looks ok
doc/source/whatsnew/v0.23.5.txt
Outdated
@@ -49,3 +49,5 @@ Bug Fixes | |||
**I/O** | |||
|
|||
- Bug in :func:`read_csv` that caused it to raise ``OverflowError`` when trying to use 'inf' as ``na_value`` with integer index column (:issue:`17128`) | |||
- Bug in :func:`to_string(index=False)` that broke column alignment (:issue:`16839`, :issue:`13032`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move to 0.24.0
can you make this more explicit, e.g. say what cases it is fixing.
assert df_s == expected | ||
|
||
def test_to_string_line_width_no_index(self): | ||
df = DataFrame({'x': [1, 2, 3], 'y': [4, 5, 6]}) | ||
|
||
df_s = df.to_string(line_width=1, index=False) | ||
expected = "x \\\n1 \n2 \n3 \n\ny \n4 \n5 \n6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment where the issues that are closed
Hello @gshiba! Thanks for updating the PR.
|
d4ac415
to
cc86cd7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks for the fix @gshiba
thanks @gshiba |
closes Justification is broken with to_string(index=False) #13032
git diff upstream/master -u -- "*.py" | flake8 --diff
'Competes' with #22437 which attempts to revert
% d
to%d
as suggested here: #13032 (comment) That turned out to affect a lot of tests, which in hindsight is expected; the% d
has been around since at least 2012 (106fe99).Instead, this PR reverts parts of #11942 and embraces the leading space even when
index=False
.df.to_string(index=False)
will print the leading space when the first column is positive only, as well as preserve leading/trailing spaces on first/last lines.With the following code:
Output with master:
Output with this PR:
Similar effect on Series as well.