REF: dataframe formatters/outputs
State: closed

- [ ] closes #xxxx
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ ] whatsnew entry

Partially addresses #36407

This is a continuation of https://github.com/pandas-dev/pandas/pull/36434.

- Separated ``StringFormatter`` (or better ``ConsoleFormatter``?) from ``DataFrameFormatter``. Placed it into the new module (subject to discussion).
- Used composition of ``DataFrameFormatter`` in ``HTMLFormatter``, ``LatexFormatter``, ``CSVFormatter`` and ``StringFormatter``. It turned out that the inheritance of each of the formatters from the base ``DataFrameFormatter`` was too complicated to comprehend. The composition seems to suit here better.
- Created new class ``DataFrameRenderer`` to keep methods for outputs in each of the formats.

This is not the ultimate refactoring, but just one more step.


Labels:
name: Refactor
description: Internal refactoring of code
name: Output-Formatting
description: __repr__ of pandas objects, to_string
name: IO CSV
description: read_csv, to_csv
name: IO HTML
description: read_html, to_html, Styler.apply, Styler.applymap
name: IO LaTeX
description: to_latex

Comments:
there's a lot going on here.  can you mark which parts are pure-cut/paste and which are not?
> 
> 
> there's a lot going on here. can you mark which parts are pure-cut/paste and which are not?

Did that partially. I hope it helps.
don't wait for me one this. prioritising reviewing 1.1.x issues and PRs
Internal error in Windows py38_np18 check.
Some weird error in travis. Looks like internal travis error with closing buffer. Locally this test runs fine.
<details>
```
____________ test_filepath_or_buffer_arg[pathlike-foo-abc-to_latex] ____________

[gw0] linux -- Python 3.7.8 /home/travis/miniconda3/envs/pandas-dev/bin/python

method = 'to_latex'

filepath_or_buffer = PosixPath('/tmp/pytest-of-travis/pytest-0/popen-gw0/test_filepath_or_buffer_arg_pa11/foo')

assert_filepath_or_buffer_equals = <function assert_filepath_or_buffer_equals.<locals>._assert_filepath_or_buffer_equals at 0x7f368ef02170>

encoding = 'foo', data = 'abc', filepath_or_buffer_id = 'pathlike'

    @pytest.mark.parametrize("method", ["to_string", "to_html", "to_latex"])

    @pytest.mark.parametrize(

        "encoding, data",

        [(None, "abc"), ("utf-8", "abc"), ("gbk", "造成输出中文显示乱码"), ("foo", "abc")],

    )

    def test_filepath_or_buffer_arg(

        method,

        filepath_or_buffer,

        assert_filepath_or_buffer_equals,

        encoding,

        data,

        filepath_or_buffer_id,

    ):

        df = DataFrame([data])

    

        if filepath_or_buffer_id not in ["string", "pathlike"] and encoding is not None:

            with pytest.raises(

                ValueError, match="buf is not a file name and encoding is specified."

            ):

                getattr(df, method)(buf=filepath_or_buffer, encoding=encoding)

        elif encoding == "foo":

            with tm.assert_produces_warning(None):

                with pytest.raises(LookupError, match="unknown encoding"):

>                   getattr(df, method)(buf=filepath_or_buffer, encoding=encoding)

pandas/tests/io/formats/test_format.py:3389: 

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <contextlib._GeneratorContextManager object at 0x7f368ef0fcd0>

type = None, value = None, traceback = None

    def __exit__(self, type, value, traceback):

        if type is None:

            try:

>               next(self.gen)

E               AssertionError: Caused unexpected warning(s): [('ResourceWarning', ResourceWarning("unclosed <ssl.SSLSocket fd=13, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('10.20.0.43', 49154), raddr=('52.217.93.220', 443)>"), '/home/travis/build/pandas-dev/pandas/pandas/core/generic.py', 5404)]
```
</details>
> Internal error in Windows py38_np18 check.
> Some weird error in travis. Looks like internal travis error with closing buffer. Locally this test runs fine.

I've restarted the jobs. probably unrelated.
> 
> 
> Thanks @ivanovmg generally lgtm.
> 
> The removal of the setters in csvs is a great improvement, for readability and static typing.
> 
> I'm not sure about removing the TableFormatter class though. This provided a common base for LatexFormatter, HTMLFormatter and what is now StringFormatter, for code that does not belong in DataFrameFormatter.
> 
> I suggest that future refactors are more atomic. I see several themes here that probably should have been discussed/reviewed in isolation. The primary one being the adoption of DataFrameFormatter for use in csv formatting. I'm also not sure that DataFrameRenderer class is even necessary.

The main target here was to extract StringFormatter from DataFrameFormatter.
And I am glad I figured out how to separate it.

Regarding ``DataFrameRenderer``, we can probably think about it later. My idea was to keep it all in one place for all formats (currently not all).

My concern with TableFormatter was that some properties (attributes) are not related to some ancestors. For example, ``is_truncated`` has nothing to do with LatexFormatter. The same for ``formatters`` and presumably ``should_show_dimensions``.

Suggestion on the atomic PRs is very well taken :) But it was really difficult to make it short when extracting StringFormatter functionality. Sorry for that.
> The main target here was to extract StringFormatter from DataFrameFormatter.
> And I am glad I figured out how to separate it.

agreed this is an important step for further refactoring. nice work.

> 
> Regarding `DataFrameRenderer`, we can probably think about it later. My idea was to keep it all in one place for all formats (currently not all).

yep. follow-ons ok.

> 
> My concern with TableFormatter was that some properties (attributes) are not related to some ancestors. For example, `is_truncated` has nothing to do with LatexFormatter. The same for `formatters` and presumably `should_show_dimensions`.

It was a bit difficult to get one's head around the Inheritance and Composition used together, and there was not a clear separation. I think that I anticipated, that over time the shared functionality in DataFrameFormatter would move to TableFormatter, DataFrameFormatter would evolve to be what is now StringFormatter and TableFormatter would evolve to be what is now DataFrameFormatter. So the latex, html and string formatters would have all inherited from a base class. But as you have noted, composition is also an option.


> 
> 
> > The main target here was to extract StringFormatter from DataFrameFormatter.
> > And I am glad I figured out how to separate it.
> 
> agreed this is an important step for further refactoring. nice work.
> 
> > Regarding `DataFrameRenderer`, we can probably think about it later. My idea was to keep it all in one place for all formats (currently not all).
> 
> yep. follow-ons ok.
> 
> > My concern with TableFormatter was that some properties (attributes) are not related to some ancestors. For example, `is_truncated` has nothing to do with LatexFormatter. The same for `formatters` and presumably `should_show_dimensions`.
> 
> It was a bit difficult to get one's head around the Inheritance and Composition used together, and there was not a clear separation. I think that I anticipated, that over time the shared functionality in DataFrameFormatter would move to TableFormatter, DataFrameFormatter would evolve to be what is now StringFormatter and TableFormatter would evolve to be what is now DataFrameFormatter. So the latex, html and string formatters would have all inherited from a base class. But as you have noted, composition is also an option.

In fact my first guess was to use inheritance. But then I realized that the ancestors will have only something in common with the parent TableFormatter. Thus, I figured out that the composition would be a better option.
Now, as the formatters are more or less separated, we can have a better look at what they have in common and probably improve the architecture (in a separate PR, once this is merged).
There are now failures related to parquet and datetime. Looks like these are persistent across multiple PRs today.
> Now, as the formatters are more or less separated, we can have a better look at what they have in common and probably improve the architecture (in a separate PR, once this is merged).

sgtm
thanks @ivanovmg