Skip to content

BUG: to_html misses truncation indicators (...) when index=False #22786

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

Merged
merged 12 commits into from
Nov 15, 2018
Merged
Prev Previous commit
Next Next commit
fix failing tests
  • Loading branch information
simonjayhawkins committed Sep 21, 2018
commit 8a82df5fa23a8687dfbaa117f2ccd1953dd9ff1f
20 changes: 17 additions & 3 deletions pandas/io/formats/html.py
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,8 @@ def _column_header():
align = self.fmt.justify

if truncate_h:
if self.fmt.index is False:
Copy link
Member

Choose a reason for hiding this comment

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

Instead of doing is False can you leverage the implicit truthiness here and do if not self.fmt.index? While not documented I believe index=0 and index=None are acceptable in this and other parsers, so would be ideal to handle those consistently with False

Copy link
Contributor

Choose a reason for hiding this comment

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

can you update this

row_levels = 0
ins_col = row_levels + self.fmt.tr_col_num
col_row.insert(ins_col, '...')

Expand Down Expand Up @@ -342,10 +344,22 @@ def _write_body(self, indent):
self._write_hierarchical_rows(fmt_values, indent)
else:
self._write_regular_rows(fmt_values, indent)
# GH 15019, GH 22783 add truncation logic below
else:
for i in range(min(len(self.frame), self.max_rows)):
row = [fmt_values[j][i] for j in range(len(self.columns))]
truncate_h = self.fmt.truncate_h
truncate_v = self.fmt.truncate_v
ncols = len(self.fmt.tr_frame.columns)
nrows = len(self.fmt.tr_frame)

row = []
Copy link
Contributor

Choose a reason for hiding this comment

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

can you give some comments on what is happening here, would not object to a self.write_row?

Copy link
Member Author

@simonjayhawkins simonjayhawkins Sep 23, 2018

Choose a reason for hiding this comment

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

@jreback
The added code is copy and pasted from self._write_regular_rows()

But for the index=False case, the (row) index value is not added to row. The assignment to dot_col_ix and the value of nindex_levels in the self.write_tr call is also changed to account for this.

Currently the truncation tests in tests\io\formats\test_to_html.py are being skipped. Without test coverage the values of dot_col_ix and nindex_levels in self._write_regular_rows() can take on any value and pass the tests.

To avoid potential regression on the notebook display codepath (index=True), i have duplicated the code rather than make any changes to self._write_regular_rows()

Tests could be added but I wanted to avoid going out of scope on this PR.

The existing code for the index=False case was not in a function unlike the code for the index=True cases. However it was only 3 lines.

So I would agree that a self.write_row function would now be a good idea if it was unlikely that a future refactoring was not to use self._write_regular_rows() instead.

How should I proceed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tests could be added but I wanted to avoid going out of scope on this PR.

can you do a pre-cursor PR which locks down the tests first?

Copy link
Contributor

Choose a reason for hiding this comment

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

@simonjayhawkins i mean comments in the code

for i in range(nrows):
if truncate_v and i == (self.fmt.tr_row_num):
str_sep_row = ['...'] * len(row)
self.write_tr(str_sep_row, indent,
self.indent_delta, tags=None)
row = [fmt_values[j][i] for j in range(ncols)]
if truncate_h:
dot_col_ix = self.fmt.tr_col_num
row.insert(dot_col_ix, '...')
self.write_tr(row, indent, self.indent_delta, tags=None)

indent -= self.indent_delta
Expand Down
12 changes: 0 additions & 12 deletions pandas/tests/io/formats/conftest.py

This file was deleted.

1 change: 0 additions & 1 deletion pandas/tests/io/formats/data/gh15019_expected_output.html
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,3 @@
</tr>
</tbody>
</table>

7 changes: 5 additions & 2 deletions pandas/tests/io/formats/test_to_html.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,13 @@


@pytest.fixture
def expected_html(read_file):
def expected_html(datapath):
"""fixture factory to read html files from tests/io/formats/data"""
def _expected_html(name):
filename = '.'.join([name, 'html'])
html = read_file(filename)
filepath = datapath('io', 'formats', 'data', filename)
with open(filepath) as f:
html = f.read()
return html.rstrip()
return _expected_html

Expand Down