BUG: to_html misses truncation indicators (...) when index=False #22786
Conversation
| Hello @simonjayhawkins! Thanks for submitting the PR.
|
| @simonjayhawkins I know you have a few overlapping PRs open at this point - is this the one you think should go in first? Want to make sure I prioritize review accordingly |
Codecov Report
@@ Coverage Diff @@ ## master #22786 +/- ## ========================================== + Coverage 92.24% 92.24% +<.01% ========================================== Files 161 161 Lines 51336 51338 +2 ========================================== + Hits 47357 47359 +2 Misses 3979 3979
Continue to review full report at Codecov.
|
jreback added Bug Output-Formatting 
pandas/io/formats/html.py Outdated
| ncols = len(self.fmt.tr_frame.columns) | ||
| nrows = len(self.fmt.tr_frame) | ||
| | ||
| row = [] |
There was a problem hiding this comment.
can you give some comments on what is happening here, would not object to a self.write_row?
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@simonjayhawkins i mean comments in the code
| can you merge master |
| @WillAyd can you have another look here |
pandas/io/formats/html.py Outdated
| align = self.fmt.justify | ||
| | ||
| if truncate_h: | ||
| if self.fmt.index is False: |
There was a problem hiding this comment.
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
pandas/io/formats/html.py Outdated
| align = self.fmt.justify | ||
| | ||
| if truncate_h: | ||
| if self.fmt.index is False: |
pandas/io/formats/html.py Outdated
| ncols = len(self.fmt.tr_frame.columns) | ||
| nrows = len(self.fmt.tr_frame) | ||
| | ||
| row = [] |
There was a problem hiding this comment.
@simonjayhawkins i mean comments in the code
| | ||
| def test_to_html_truncation_index_false_max_rows(self, datapath): | ||
| # GH 15019 | ||
| np.random.seed(seed=0) |
There was a problem hiding this comment.
do really need to set the seed?
| # GH 15019 | ||
| np.random.seed(seed=0) | ||
| df = pd.DataFrame(np.random.randn(5, 2)) | ||
| result = df.to_html(max_rows=4, index=False) |
There was a problem hiding this comment.
can you parameterize over index=False and index=0
| def expected_html(datapath, name): | ||
| """ | ||
| Read HTML file from formats data directory. | ||
There was a problem hiding this comment.
might be nice to change some of the existing tests to use this function (future PR though)
There was a problem hiding this comment.
PR #23747 to change existing tests to use this function.
| @simonjayhawkins code lgtm. Can you resolve conflicts? Can merge thereafter on green |
5 participants
git diff upstream/master -u -- "*.py" | flake8 --diff