Skip to content
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

DOC: Updating str_pad docstring #22570

Merged
merged 13 commits into from
Sep 25, 2018

Conversation

JesperDramsch
Copy link
Contributor

Added Examples to docstring.
Added documentation to each part of docstring

@codecov
Copy link

codecov bot commented Sep 1, 2018

Codecov Report

Merging #22570 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #22570      +/-   ##
==========================================
+ Coverage   92.04%   92.18%   +0.14%     
==========================================
  Files         169      169              
  Lines       50787    50810      +23     
==========================================
+ Hits        46745    46839      +94     
+ Misses       4042     3971      -71
Flag Coverage Δ
#multiple 90.6% <100%> (+0.14%) ⬆️
#single 42.37% <0%> (+0.08%) ⬆️
Impacted Files Coverage Δ
pandas/core/strings.py 98.63% <100%> (-0.01%) ⬇️
pandas/io/clipboard/clipboards.py 28.23% <0%> (-2.36%) ⬇️
pandas/compat/__init__.py 58.43% <0%> (-0.18%) ⬇️
pandas/core/indexes/base.py 96.39% <0%> (-0.06%) ⬇️
pandas/core/dtypes/inference.py 98.38% <0%> (-0.03%) ⬇️
pandas/core/indexes/period.py 93.48% <0%> (-0.02%) ⬇️
pandas/core/indexes/category.py 97.26% <0%> (-0.02%) ⬇️
pandas/core/frame.py 97.2% <0%> (-0.01%) ⬇️
pandas/core/indexes/accessors.py 90.09% <0%> (ø) ⬆️
... and 48 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 98fb53c...c785311. Read the comment docs.

@gfyoung gfyoung added Docs Strings String extension data type and string data labels Sep 1, 2018
pandas/core/strings.py Outdated Show resolved Hide resolved
Copy link
Member

@datapythonista datapythonista 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, added couple of comments.

pandas/core/strings.py Outdated Show resolved Hide resolved
pandas/core/strings.py Outdated Show resolved Hide resolved
pandas/core/strings.py Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Sep 3, 2018

Hello @JesperDramsch! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on September 03, 2018 at 17:10 Hours UTC

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Nice docstring, added couple of comments that I think could improve it further.

pandas/core/strings.py Outdated Show resolved Hide resolved
pandas/core/strings.py Outdated Show resolved Hide resolved
@@ -1314,21 +1314,65 @@ def str_index(arr, sub, start=0, end=None, side='left'):

def str_pad(arr, width, side='left', fillchar=' '):
"""
Pad strings in the Series/Index with an additional character to
specified side.
Pad strings in the Series/Index up to width.

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 in this case we could have a extended summary, providing a bit more information on what pad means, and when it can be useful.

@jreback
Copy link
Contributor

jreback commented Sep 18, 2018

@datapythonista

@datapythonista
Copy link
Member

@JesperDramsch do you have time to address the previous comments?

@JesperDramsch
Copy link
Contributor Author

@JesperDramsch do you have time to address the previous comments?

I'll get to it this weekend, I think.

@JesperDramsch
Copy link
Contributor Author

I think I addressed all comments and validated it. @datapythonista

Copy link
Member

@datapythonista datapythonista left a comment

Choose a reason for hiding this comment

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

Just couple of additional comments. Besides those, looks great.

See Also
--------
Series.str.rjust: Fills the left side of strings with an arbitrary
character. Equivalent to pd.str.pad(side='left').
Copy link
Member

Choose a reason for hiding this comment

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

it's not really pd.str.pad, but Series.str.pad. Can you also add double backticks around it so it renders in the website as code. Same for all cases.


Parameters
----------
width : int
Minimum width of resulting string; additional characters will be filled
with spaces
with character defined in fillchar.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add backticks around fillchar, so it's explicit that is the parameter name.

@jorisvandenbossche jorisvandenbossche added this to the 0.24.0 milestone Sep 25, 2018
@jorisvandenbossche jorisvandenbossche merged commit a936399 into pandas-dev:master Sep 25, 2018
@jorisvandenbossche
Copy link
Member

@JesperDramsch Pushed some last small changes. Thanks a lot for the PR!

@JesperDramsch JesperDramsch deleted the str_pad-docstring branch September 25, 2018 17:16
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Strings String extension data type and string data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants