Skip to content

DOC: split docstring into multiple lines in excel.py #14073

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 2 commits into from
Sep 15, 2016
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions pandas/io/excel.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
from warnings import warn
from distutils.version import LooseVersion
from pandas.util.decorators import Appender
from textwrap import fill

__all__ = ["read_excel", "ExcelWriter", "ExcelFile"]

Expand Down Expand Up @@ -97,7 +98,7 @@
na_values : scalar, str, list-like, or dict, default None
Additional strings to recognize as NA/NaN. If dict passed, specific
per-column NA values. By default the following values are interpreted
as NaN: '""" + "', '".join(sorted(_NA_VALUES)) + """'.
as NaN: '""" + fill("', '".join(sorted(_NA_VALUES)), 80) + """'.
Copy link
Member

Choose a reason for hiding this comment

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

I would take a lower number than 80. We already have the indentation of 4 chars in the docstring itself, so maximum 76. But I would maybe take just 70 or so

thousands : str, default None
Thousands separator for parsing string columns to numeric. Note that
this parameter is only necessary for columns stored as TEXT in Excel,
Expand Down Expand Up @@ -168,7 +169,20 @@ def get_writer(engine_name):
raise ValueError("No Excel writer '%s'" % engine_name)


@Appender(_read_excel_doc)
def split_doc(doc_string, word_wrap=80):
Copy link
Contributor

@jreback jreback Aug 25, 2016

Choose a reason for hiding this comment

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

Appender already calls dedent internally, I don't think you could add make it also call wrap here and it would just work. (the width could be an optional parameter to the Appender constructor).

But we will have to check some doc strings.

@jorisvandenbossche

Copy link
Member

Choose a reason for hiding this comment

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

I think the how it is here done is fine. In principle we could add a check for this in Appender, but this is also fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

no this is adding code which is the same as wrap
otherwise it needs to be in a common location

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, moving it to a common location is better, I just meant the current function (after moving) is fine.

wrap is not doing exactly the same as this does not take account of the indentation needed in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

ok but moving to Appender is pretty trivially and he obvious location

doc_lines = doc_string.split('\n')
doc_string = ''
for line in doc_lines:
if len(line) > word_wrap:
break_pos = line[:word_wrap].rfind(' ')
indent_width = len(line) - len(line.lstrip())
line = line[:break_pos] + '\n' + \
' ' * indent_width + line[break_pos + 1:]
doc_string += (line + '\n')
return doc_string


@Appender(split_doc(_read_excel_doc))
def read_excel(io, sheetname=0, header=0, skiprows=None, skip_footer=0,
index_col=None, names=None, parse_cols=None, parse_dates=False,
date_parser=None, na_values=None, thousands=None,
Expand Down