CLN: io/formats/html.py: refactor #22726

simonjayhawkins · 2018-09-16T01:04:57Z

[N/A ] follow on from Preliminary format refactor #20341
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
[N/A ] whatsnew entry

pep8speaks · 2018-09-16T01:04:58Z

Hello @simonjayhawkins! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/io/formats/html.py !

gfyoung · 2018-09-16T04:58:56Z

@simonjayhawkins : Not sure where this was discussed before, but could you explain in your PR description why making indent a stateful entity makes sense than leaving it as a parameter to individual methods?

simonjayhawkins · 2018-09-16T06:21:50Z

@gfyoung : good question. why change?

at the moment this was just an exercise to see if the current behavior could be implemented in a simpler way. why?

was thinking what would the code look like if the common behavior of to_string()/DataFrameFormatter and to_html()/HTMLFormatter were inherited from TableFormatter.

was also thinking how would the code look if to_html(notebook=True) were a subclass of HTMLFormatter, say NotebookFormatter and to_html(notebook=False) were also a subclass, say ToHTMLFormatter.

i think this would allow easier implementation of some of the enhancements commonly requested for the to_html() method without making potentially breaking changes to the notebook display code path.
for example, #22692 just in the last few days,

with regard to fine grain control of the indentation on a method level, another subclass of HTMLFormatter maybe. but that is not needed at the moment as the current indentation behavior can be replicated without it.

i'll report back when i have explored this avenue further.

gfyoung · 2018-09-16T07:32:56Z

i'll report back when i have explored this avenue further.

Sounds good. Might have preferred that an issue be filed first, but admittedly, the discussion would have been hard to carry without something concrete to analyse.

simonjayhawkins · 2018-09-16T17:57:28Z

quote from #20341

... PR splits the various formatters into different files so that it's easier to refactor a specific formatter.

simonjayhawkins · 2018-09-17T14:15:52Z

@gfyoung : i've reverted the changes to indentation for the moment and added some simpler refactorings to start.

gfyoung · 2018-09-18T02:52:43Z

pandas/io/formats/html.py

@@ -79,7 +79,7 @@ def _write_cell(self, s, kind='td', indent=0, tags=None):
        self.write(u'{start}{rs}</{kind}>'
                   .format(start=start_tag, rs=rs, kind=kind), indent)

-    def write_tr(self, line, indent=0, indent_delta=4, header=False,
+    def write_tr(self, line, indent=0, indent_delta=0, header=False,


Changing the default argument is an API change. Could you explain why you are doing this?

I think that the only public method of HTMLFormatter should be write_result().

The other methods are currently a mix of private and public. There does not seem to be any consistency.

If write_tr should be private, the API change would not be an issue. The indent_delta is not left to the default by any calls in the current implementation. indent_delta=0 would be a cleaner default.

Following on from the discussion regarding making indent a stateful entity rather than leaving it as a parameter to individual methods: The write_tr method accesses the bold_rows instance variable. The value should probably be passed through the method's nindex_levels parameter which was probably the intention of adding that parameter to the method.

The other HTML formatting methods write, write_th, write_td and _write_cell do not access instance variables with the exception that write needs access to elements which effectively the output buffer.

Also should have said: the use of bold_rows in write_tr is why I stopped refactoring at that point for the moment. I'm not sure how many changes should be in one PR.

I've added some additional tests, but not all paths can be tested until the fixes for #22655 have been implemented.

yeah this is ok, as its internal. I agree its less that clear what is external / internal (and we don't usually use _ prefixed). If you want to indicate in the doc-string of an external API would be ok.

jreback · 2018-09-18T11:10:03Z

does this actually change any output?

simonjayhawkins · 2018-09-18T11:39:21Z

@jreback : Are you referring to the comment regarding indent_delta, or the removal of the unreachable code and subsequent refactoring? The answer is no and no.

I've added some additional tests, but not all paths can be tested until the fixes for #22655 have been implemented.

All the tests in pandas\tests\io\formats\test_to_html.py currently test the full df.to_html() path. Adding additional tests is quite time consuming. I still need to add some more tests to #22655 before I'm happy that the permutations of input parameters with DataFrame truncation has been fully tested.

maybe the way forward is to add unit tests for the HTMLFormatter methods?

jreback · 2018-09-18T11:51:12Z

lgtm. @WillAyd if you'd have a quick look.

pandas/tests/io/formats/test_to_html.py

WillAyd · 2018-09-19T19:43:22Z

pandas/tests/io/formats/test_to_html.py

+            _has_names(index))
+
+
+@pytest.fixture


Is this going to be used outside this module? If not then can probably just be a global function instead of a fixture. If so then I think would make more sense to move fixtures to a conftest for this package

It will not be of use outside this module. It uses read_file which uses the datapath fixture. So does it not need to be a fixture or am I doing something wrong?

Hmm I think I misread this originally. Are you expecting to use this fixture in any of the test methods or just in the expected_output fixture? If the latter it might just make more sense to have this all in one fixture so that this doesn't get unintentionally used later

I think it would be a good idea if the methodology used here, if approved, is then used to move all the inline HTML from the other tests into separate files. Would be easier to check the expected output of other tests. Would be easier to re-use the expect outputs of other tests. And would allow much larger outputs/comprehensive tests to be used in future tests without cluttering the test file.

I'll move it all into one fixture for the moment. It can always be separated out again in the future.

In #22655, I've added a issue specific test as well as the parametric regression tests. So I am already using expected_html in more than one test and will need to leave it as it is.

WillAyd · 2018-09-19T19:44:16Z

pandas/tests/io/formats/test_to_html.py

+
+
+@pytest.fixture
+def expected_output(expected_html):


Similar comment as for expected_html

codecov · 2018-09-19T22:19:04Z

Codecov Report

Merging #22726 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #22726      +/-   ##
==========================================
+ Coverage   92.23%   92.26%   +0.02%     
==========================================
  Files         161      161              
  Lines       51408    51387      -21     
==========================================
- Hits        47416    47410       -6     
+ Misses       3992     3977      -15

Flag	Coverage Δ
#multiple	`90.65% <100%> (+0.02%)`	⬆️
#single	`42.32% <0%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
pandas/io/formats/html.py	`97.68% <100%> (+4.49%)`	⬆️

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 1250500...0905033. Read the comment docs.

jreback · 2018-11-06T03:31:30Z

can you merge master on this.

jreback · 2018-11-06T03:31:38Z

@WillAyd comments?

WillAyd · 2018-11-06T06:18:10Z

Been a little tight for time recently but should be able to review again before the end of the week. @simonjayhawkins if you could merge master in the interim would be helpful

WillAyd

Generally looks good - one question around the handling of MI but overall a lot simpler. Nice job!

WillAyd · 2018-11-07T06:28:31Z

pandas/io/formats/html.py

-                    row.append(single_column_table(self.columns.names))
-                else:
-                    row.append('')
-                style = "text-align: {just};".format(just=self.fmt.justify)


Relatively nuanced but in the case of a MultiIndex isn't this formatting the very first column row different from the rest? If so is that accounted for in the refactor?

This if clause is unreachable since _column_header() was only called from the else clause of another if isinstance(self.columns, ABCMultiIndex): so the styling is not accounted for in this refactor.

Makes sense thanks for clarifying

WillAyd

@jreback back to you

jreback · 2018-11-18T22:32:03Z

thanks @simonjayhawkins

…fixed * upstream/master: (46 commits) DEPS: bump xlrd min version to 1.0.0 (pandas-dev#23774) BUG: Don't warn if default conflicts with dialect (pandas-dev#23775) BUG: Fixing memory leaks in read_csv (pandas-dev#23072) TST: Extend datetime64 arith tests to array classes, fix several broken cases (pandas-dev#23771) STYLE: Specify bare exceptions in pandas/tests (pandas-dev#23370) ENH: between_time, at_time accept axis parameter (pandas-dev#21799) PERF: Use is_utc check to improve performance of dateutil UTC in DatetimeIndex methods (pandas-dev#23772) CLN: io/formats/html.py: refactor (pandas-dev#22726) API: Make Categorical.searchsorted returns a scalar when supplied a scalar (pandas-dev#23466) TST: Add test case for GH14080 for overflow exception (pandas-dev#23762) BUG: Don't extract header names if none specified (pandas-dev#23703) BUG: Index.str.partition not nan-safe (pandas-dev#23558) (pandas-dev#23618) DEPR: tz_convert in the Timestamp constructor (pandas-dev#23621) PERF: Datetime/Timestamp.normalize for timezone naive datetimes (pandas-dev#23634) TST: Use new arithmetic fixtures, parametrize many more tests (pandas-dev#23757) REF/TST: Add more pytest idiom to parsers tests (pandas-dev#23761) DOC: Add ignore-deprecate argument to validate_docstrings.py (pandas-dev#23650) ENH: update pandas-gbq to 0.8.0, adds credentials arg (pandas-dev#23662) DOC: Improve error message to show correct order (pandas-dev#23652) ENH: Improve error message for empty object array (pandas-dev#23718) ...

refactor indentation

63605b2

gfyoung added IO HTML read_html, to_html, Styler.apply, Styler.applymap Clean Testing pandas testing functions or related to the test suite Refactor Internal refactoring of code and removed Testing pandas testing functions or related to the test suite Clean labels Sep 16, 2018

simonjayhawkins added 6 commits September 17, 2018 14:39

undo indentation changes

d0dd432

add tests from pandas-devgh-22655 (not merged)

a7db256

Merge remote-tracking branch 'upstream/master' into indentation

30d9dd5

remove unreachable code block

98104a7

remove unused functions

f8a6f8e

refactor _column_header()

61c3c19

simonjayhawkins changed the title ~~CLN: io/formats/html.py: refactor indentation~~ CLN: io/formats/html.py: refactor Sep 17, 2018

simonjayhawkins added 2 commits September 17, 2018 15:03

inline code from _column_header()

cb394f2

change default indent_delta for write_tr()

a6ce459

gfyoung reviewed Sep 18, 2018

View reviewed changes

jreback added this to the 0.24.0 milestone Sep 18, 2018

WillAyd requested changes Sep 19, 2018

View reviewed changes

remove unapproved tests

b65bb53

simonjayhawkins mentioned this pull request Sep 20, 2018

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

Merged

5 tasks

Merge remote-tracking branch 'upstream/master' into indentation

af23095

WillAyd requested changes Nov 7, 2018

View reviewed changes

WillAyd approved these changes Nov 8, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into indentation

0905033

simonjayhawkins mentioned this pull request Nov 18, 2018

BUG: output formatting with to_html(), index=False and/or index_names=False (#22579, #22747) #22655

Merged

5 tasks

jreback merged commit cc712c9 into pandas-dev:master Nov 18, 2018

simonjayhawkins deleted the indentation branch November 19, 2018 00:14

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

CLN: io/formats/html.py: refactor (pandas-dev#22726)

c3ddcf2

simonjayhawkins mentioned this pull request Nov 20, 2018

CLN: io/formats/html.py: refactor #23818

Merged

3 tasks

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CLN: io/formats/html.py: refactor (pandas-dev#22726)

7aecb97

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

CLN: io/formats/html.py: refactor (pandas-dev#22726)

a626343

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLN: io/formats/html.py: refactor #22726

CLN: io/formats/html.py: refactor #22726

simonjayhawkins commented Sep 16, 2018 •

edited

Loading

pep8speaks commented Sep 16, 2018

gfyoung commented Sep 16, 2018

simonjayhawkins commented Sep 16, 2018

gfyoung commented Sep 16, 2018 •

edited

Loading

simonjayhawkins commented Sep 16, 2018

simonjayhawkins commented Sep 17, 2018

gfyoung Sep 18, 2018

simonjayhawkins Sep 18, 2018

simonjayhawkins Sep 18, 2018

jreback Sep 18, 2018

jreback commented Sep 18, 2018

simonjayhawkins commented Sep 18, 2018 •

edited

Loading

jreback commented Sep 18, 2018

WillAyd Sep 19, 2018

simonjayhawkins Sep 19, 2018

WillAyd Sep 19, 2018

simonjayhawkins Sep 19, 2018

simonjayhawkins Sep 19, 2018

WillAyd Sep 19, 2018

codecov bot commented Sep 19, 2018 •

edited

Loading

jreback commented Nov 6, 2018

jreback commented Nov 6, 2018

WillAyd commented Nov 6, 2018

WillAyd left a comment

WillAyd Nov 7, 2018

simonjayhawkins Nov 8, 2018

WillAyd Nov 8, 2018

WillAyd left a comment

jreback commented Nov 18, 2018

		_has_names(index))


		@pytest.fixture

CLN: io/formats/html.py: refactor #22726

CLN: io/formats/html.py: refactor #22726

Conversation

simonjayhawkins commented Sep 16, 2018 • edited Loading

pep8speaks commented Sep 16, 2018

gfyoung commented Sep 16, 2018

simonjayhawkins commented Sep 16, 2018

gfyoung commented Sep 16, 2018 • edited Loading

simonjayhawkins commented Sep 16, 2018

simonjayhawkins commented Sep 17, 2018

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

jreback commented Sep 18, 2018

simonjayhawkins commented Sep 18, 2018 • edited Loading

jreback commented Sep 18, 2018

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

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2018 • edited Loading

Codecov Report

jreback commented Nov 6, 2018

jreback commented Nov 6, 2018

WillAyd commented Nov 6, 2018

WillAyd left a 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 left a comment

Choose a reason for hiding this comment

jreback commented Nov 18, 2018

simonjayhawkins commented Sep 16, 2018 •

edited

Loading

gfyoung commented Sep 16, 2018 •

edited

Loading

simonjayhawkins commented Sep 18, 2018 •

edited

Loading

codecov bot commented Sep 19, 2018 •

edited

Loading