Skip to content

CLN: Preliminary formatter refactor #20051

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

Conversation

shangyian
Copy link
Contributor

  • based on discussion here: Latex bugs #20032 (review)
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

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

@shangyian shangyian mentioned this pull request Mar 8, 2018
4 tasks
@jreback jreback added Output-Formatting __repr__ of pandas objects, to_string Clean labels Mar 8, 2018
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks good. just some very minor comments. want to do this refactor w/o really changing much code (as its more transparent that way). next pass will clean / fix more things.


def buffer_put_lines(buf, lines):
"""
Appends lines to a buffer
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 use numpy doc string format here

longtable : boolean, default False
Use a longtable environment instead of tabular.

See also
Copy link
Contributor

Choose a reason for hiding this comment

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

See Also

OrderedDict, unichr)
from pandas.core.config import get_option
from pandas.io.formats.printing import pprint_thing
from pandas.io.formats.common import get_level_lengths, \
Copy link
Contributor

Choose a reason for hiding this comment

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

use parens rather than line continuation \

from pandas.core.indexes.datetimes import DatetimeIndex
from pandas.core.indexes.period import PeriodIndex
import numpy as np

Copy link
Contributor

Choose a reason for hiding this comment

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

put these import first (csv then numpy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, just fixed these

@jreback jreback added IO CSV read_csv, to_csv IO HTML read_html, to_html, Styler.apply, Styler.applymap IO LaTeX to_latex labels Mar 8, 2018
@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #20051 into master will increase coverage by <.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #20051      +/-   ##
==========================================
+ Coverage   91.72%   91.73%   +<.01%     
==========================================
  Files         150      152       +2     
  Lines       49122    49180      +58     
==========================================
+ Hits        45057    45114      +57     
- Misses       4065     4066       +1
Flag Coverage Δ
#multiple 90.11% <93.75%> (ø) ⬆️
#single 41.79% <18.59%> (-0.07%) ⬇️
Impacted Files Coverage Δ
pandas/io/formats/latex.py 100% <100%> (ø)
pandas/io/formats/excel.py 97.36% <100%> (ø) ⬆️
pandas/core/frame.py 97.18% <100%> (ø) ⬆️
pandas/io/formats/html.py 88.85% <88.85%> (ø)
pandas/io/formats/format.py 98.24% <96.15%> (+1.98%) ⬆️
pandas/io/formats/csvs.py 98.05% <98.05%> (ø)
pandas/core/base.py 96.78% <0%> (-0.02%) ⬇️
pandas/core/indexes/datetimes.py 95.64% <0%> (-0.01%) ⬇️
pandas/core/series.py 93.85% <0%> (-0.01%) ⬇️
pandas/core/groupby.py 92.14% <0%> (-0.01%) ⬇️
... and 12 more

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 7c14e4f...c9b4504. Read the comment docs.

@shangyian shangyian closed this Mar 8, 2018
@shangyian shangyian reopened this Mar 8, 2018
@shangyian
Copy link
Contributor Author

Hmm. Not sure why Travis failed and AppVeyor succeeded.

@shangyian shangyian closed this Mar 9, 2018
@shangyian shangyian reopened this Mar 9, 2018
@jreback
Copy link
Contributor

jreback commented Mar 10, 2018

you have a lint error

inside ci/lint.sh
Linting *.py
pandas/io/formats/format.py:33:1: F401 'pandas.io.formats.csv_format.CSVFormatter' imported but unused
Linting *.py DONE

@jreback
Copy link
Contributor

jreback commented Mar 10, 2018

ok I fixed up the imports a bit to make them more idiomatic. I think its possible to completely remove pandas.io.formats.common and just move these 2 functions back to .formats. No real reason to have an extra file here. see if you can do that.

@jreback jreback changed the title Preliminary formatter refactor CLN: Preliminary formatter refactor Mar 10, 2018
@jreback jreback added this to the 0.23.0 milestone Mar 10, 2018
@jreback
Copy link
Contributor

jreback commented Mar 10, 2018

thanks @shangyian

will merge a bit later (after doc sprint)

@shangyian
Copy link
Contributor Author

@jreback - cool. I also moved the two functions in io.formats.common to formats.format

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

great. there is also a _get_level_lengths in style.py, you might be able to combine these (can do here or next PR)

@@ -1841,7 +1842,7 @@ def info(self, verbose=None, buf=None, max_cols=None, memory_usage=None,
- If False, never show counts.

"""
from pandas.io.formats.format import _put_lines
from pandas.io.formats.format import buffer_put_lines
Copy link
Contributor

Choose a reason for hiding this comment

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

you can reference this directly as fmt.buffer_put_lines as fmt is already imported

@@ -1940,7 +1941,7 @@ def _sizeof_fmt(num, size_qualifier):
mem_usage = self.memory_usage(index=True, deep=deep).sum()
lines.append("memory usage: %s\n" %
_sizeof_fmt(mem_usage, size_qualifier))
_put_lines(buf, lines)
buffer_put_lines(buf, lines)
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed those. I'll take a look at the two get_level_lengths, but may not get around to fixing that today, so it's probably better as a new PR

@jreback
Copy link
Contributor

jreback commented Mar 13, 2018

can you rebase on master. ping on green.

@shangyian shangyian closed this Mar 14, 2018
@shangyian shangyian force-pushed the prelim_format_refactor branch from c9b4504 to 3783ccc Compare March 14, 2018 08:32
@jorisvandenbossche jorisvandenbossche merged commit 3783ccc into pandas-dev:master Mar 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean IO CSV read_csv, to_csv IO HTML read_html, to_html, Styler.apply, Styler.applymap IO LaTeX to_latex Output-Formatting __repr__ of pandas objects, to_string
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants