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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 15, 2016
Merged

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

merged 2 commits into from
Sep 15, 2016

Conversation

OXPHOS
Copy link
Contributor

@OXPHOS OXPHOS commented Aug 23, 2016

A follow-up on #14030.
Not sure whether this is the good way to do it - this seems too verbose.
I will add it to parsers.py or move it to common.py once it's done.

I also thought about moving the function to Appender to fix the length problem for good if necessary.

@OXPHOS OXPHOS changed the title DOC: split docstring into multiple lines in excel.py DOC: split docstring into multiple lines in excel.py Aug 23, 2016
@OXPHOS OXPHOS changed the title DOC: split docstring into multiple lines in excel.py DOC: split docstring into multiple lines in excel.py Aug 23, 2016
@jreback
Copy link
Contributor

jreback commented Aug 23, 2016

this is also an issue on read_csv doc string as well

@codecov-io
Copy link

codecov-io commented Aug 23, 2016 •
edited
Loading

Current coverage is 85.24% (diff: 100%)

Merging #14073 into master will decrease coverage by 0.02%

@@             master     #14073   diff @@
==========================================
  Files           139        140     +1   
  Lines         50511      50559    +48   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43071      43099    +28   
- Misses         7440       7460    +20   
  Partials          0          0

Powered by Codecov. Last update 10bf721...654feea

@OXPHOS
Copy link
Contributor Author

OXPHOS commented Aug 24, 2016

@jreback do u want me to move the func to common.py?

@jreback jreback added the Docs label Aug 25, 2016
@jreback jreback added this to the 0.19.0 milestone Aug 25, 2016
@jreback
Copy link
Contributor

jreback commented Aug 25, 2016

yes move to common.py

jreback
jreback reviewed Aug 25, 2016
View reviewed changes
pandas/io/excel.py
@@ -168,7 +168,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 •
edited
Loading

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

@OXPHOS
Copy link
Contributor Author

OXPHOS commented Aug 30, 2016

@jreback @jorisvandenbossche I assume this is what you're talking about? It seems okay to me.
I have limited access to Internet in these few days so I might not responding in time.

@jorisvandenbossche
Copy link
Member

@OXPHOS Just a question, what did you precisely change regarding to the previous version (I just don't remember exactly anymore, sorry). Sometimes adding a commit instead of amending the previous can make this more clearer.

@OXPHOS
Copy link
Contributor Author

OXPHOS commented Aug 31, 2016

@jorisvandenbossche oh sorry my bad. I realized this problem but made the wrong fix.
So I was doing this only for the last commit.

And now I used this

@jorisvandenbossche
Copy link
Member

@OXPHOS coming back to this, is the spit_doc function still needed now that you added a call to textwrap.fill (which already splits the long string created by "', '".join(sorted(_NA_VALUES)) ? Seems like you can remove this now?

jorisvandenbossche
jorisvandenbossche reviewed Sep 14, 2016
View reviewed changes
pandas/io/excel.py
@@ -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

@OXPHOS
Copy link
Contributor Author

OXPHOS commented Sep 14, 2016

@jorisvandenbossche I updated the wrap char number and now both parses.py and excel.py uses textwrap.fill

@jorisvandenbossche
Copy link
Member

@OXPHOS Thanks a lot, looking good!

@jorisvandenbossche jorisvandenbossche merged commit 5e2f9da into pandas-dev:master Sep 15, 2016
yarikoptic added a commit to neurodebian/pandas that referenced this pull request Oct 13, 2016
@yarikoptic
Merge commit 'v0.19.0rc1-25-ga7469cf' into debian
ea7aee4 
* commit 'v0.19.0rc1-25-ga7469cf': (471 commits)
  ENH: Add divmod to series and index. (pandas-dev#14208)
  Fix generator tests to run (pandas-dev#14245)
  BUG: GH13629 Binned groupby median function with empty bins (pandas-dev#14225)
  TST/TEMP: fix pyqt to 4.x for plotting tests (pandas-dev#14240)
  DOC: added example to Series.map showing use of na_action parameter (GH14231)
  DOC: split docstring into multiple lines in excel.py (pandas-dev#14073)
  MAINT: Use __module__ in _DeprecatedModule. (pandas-dev#14181)
  ENH: Allow true_values and false_values options in read_excel (pandas-dev#14002)
  DOC: fix incorrect example in unstack docstring (GH14206) (pandas-dev#14211)
  BUG: iloc fails with non lex-sorted MultiIndex pandas-dev#13797
  BUG: add check for infinity in __call__ of EngFormatter
  In gbq.to_gbq allow the DataFrame column order to differ from schema
  BLD: require cython if tempita is needed
  DOC: add source links to api docs (pandas-dev#14200)
  BUG: compat with Stata ver 111
  Fix: F999 dictionary key '2000q4' repeated with different values (pandas-dev#14198)
  BLD: Test for Python 3.5 with C locale
  BUG: DatetimeTZBlock can't assign values near dst boundary
  BUG: union_categorical with Series and cat idx
  BUG: fix str.contains for series containing only nan values
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisvandenbossche jorisvandenbossche jorisvandenbossche approved these changes

Assignees
No one assigned
Labels
Docs
Projects
None yet
Milestone
0.19.0
Development

Successfully merging this pull request may close these issues.

DOC: read_excel() documentation for na_values should show default na values
4 participants
@OXPHOS @jreback @codecov-io @jorisvandenbossche