DOC: Improve the docstring of String.str.zfill() #20864

alkamid · 2018-04-29T11:25:24Z

closes #xxxx
tests added / passed
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

Result of the Pandas sprint at PyData London 2018. Includes work by @owlas.

codecov · 2018-04-29T13:42:26Z

Codecov Report

❗ No coverage uploaded for pull request base (master@876e568). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #20864   +/-   ##
=========================================
  Coverage          ?   91.81%           
=========================================
  Files             ?      153           
  Lines             ?    49478           
  Branches          ?        0           
=========================================
  Hits              ?    45429           
  Misses            ?     4049           
  Partials          ?        0

Flag	Coverage Δ
#multiple	`90.21% <ø> (?)`
#single	`41.85% <ø> (?)`

Impacted Files	Coverage Δ
pandas/core/strings.py	`98.63% <ø> (ø)`

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 876e568...706688f. Read the comment docs.

datapythonista

Great job, added some comments with typos and ideas.

datapythonista · 2018-04-30T18:53:05Z

pandas/core/strings.py

-        Equivalent to :meth:`str.zfill`.
+        Pad strings in the Series/Index by prepending '0' characters.
+
+        Strings in the Series/Index are padded with prepending '0' characeters


typo in characters

datapythonista · 2018-04-30T18:56:00Z

pandas/core/strings.py

+        Strings in the Series/Index are padded with prepending '0' characeters
+        (i.e. on the left of the string) to reach a total string length
+        of `width`.  in the Series/Index with length greater than `width`
+        are unchanged.


I think something went wrong with that last sentence, two spaces, not starting with a capital letter, and I think it's not that clear, a word or something is missing I guess.

datapythonista · 2018-04-30T19:07:52Z

pandas/core/strings.py

+        are unchanged.
+
+        Note: Differs from :meth:`str.zfill` which has special handling
+        for '+'/'-' in the string.


There is a Notes section in the numpy docstring convention that we can use for this:

Notes -------

https://python-sprints.github.io/pandas/guide/pandas_docstring.html#section-6-notes

datapythonista · 2018-04-30T19:09:39Z

pandas/core/strings.py

-            Minimum width of resulting string; additional characters will be
-            filled with 0
+            Minimum length of resulting string; strings with length less
+            than `width` be prepended with '0' characters.

        Returns
        -------
        filled : Series/Index of objects


you can get rid of the filled and just leave the type (it was usual to name the return, but it was decided to not name them anymore as it doesn't add much value).

datapythonista · 2018-04-30T19:10:33Z

pandas/core/strings.py

        """
+


no need to add this blank line here

datapythonista · 2018-04-30T19:15:02Z

pandas/core/strings.py

+        >>> s.str.zfill(5)
+        0    000-2
+        1    00005
+        dtype: object


the example looks cool, but I think we can make it a bit more compact and clear by:

Just creating a single Series (I'd get rid of one of 10 or 127 as they show the same user case, and I'd add a NaN and a number (as number, not as string).

After creating the Series I'd show it without modification (i.e. >>> s). This makes the result easier to compare with the original data.

We can get rid of the second example if we add the numbers in the first.

pep8speaks · 2018-04-30T21:25:48Z

Hello @alkamid! Thanks for updating the PR.

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

Comment last updated on July 07, 2018 at 14:20 Hours UTC

alkamid · 2018-04-30T21:27:43Z

@datapythonista thanks a lot for your valuable feedback! I've addressed all your comments, does it look better to you now?

datapythonista · 2018-05-01T09:19:36Z

pandas/core/strings.py

+        4       127
+        5       NaN
+        dtype: object
+        >>> s.str.zfill(5)


Looks great now. Just couple of ideas, feel free to ignore them if you prefer the way it is now.

I think it could be useful to add a short paragraph before this line explaining that the non-string values (127 and NaN) become NaN, and may be reemphasizing what you already have in the body of the docstring, that the values with sign (-2 and +5) get the zeros in the left of the sign, and that 423523 keeps unchanged as it's longer than width.

I also think that it would make things easier/faster to understand if the values look a bit less arbitrary. For example ['-1', '1', 10, np.nan, '1000'] (with a width of 3 in this example).

datapythonista · 2018-05-01T09:19:50Z

pandas/core/strings.py

        """
+


No need to add this blank line.

…str_zfill

alkamid · 2018-05-02T21:33:02Z

@datapythonista thanks again, suggestions incorporated into PR. Let me know what you think now.

datapythonista

lgtm

…_series_str_zfill

alkamid added 2 commits April 29, 2018 12:19

DOC: Improve the docstring of String.str.zfill()

1e16540

Fix PEP8 errors

b89cd54

owlas mentioned this pull request Apr 29, 2018

Improve docstring for zfill #20861

Closed

4 tasks

datapythonista reviewed Apr 30, 2018

View reviewed changes

alkamid added 2 commits April 30, 2018 21:55

Merge branch 'master' into docstring_series_str_zfill

6669889

Correct language, add more concise example

07f4189

Fix PEP8 errors

8034856

datapythonista reviewed May 1, 2018

View reviewed changes

alkamid added 2 commits May 2, 2018 22:29

Explain behaviour in the example

60ea781

Merge remote-tracking branch 'upstream/master' into docstring_series_…

4a54a66

…str_zfill

datapythonista approved these changes May 2, 2018

View reviewed changes

jreback added Docs Strings String extension data type and string data labels May 4, 2018

Merge remote-tracking branch 'upstream/master' into alkamid-docstring…

706688f

…_series_str_zfill

TomAugspurger added this to the 0.24.0 milestone Jul 7, 2018

TomAugspurger merged commit 3344918 into pandas-dev:master Jul 7, 2018

alkamid deleted the docstring_series_str_zfill branch July 7, 2018 14:59

Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018

DOC: Improve the docstring of String.str.zfill() (pandas-dev#20864)

6bcae88

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOC: Improve the docstring of String.str.zfill() #20864

DOC: Improve the docstring of String.str.zfill() #20864

alkamid commented Apr 29, 2018

codecov bot commented Apr 29, 2018 •

edited

Loading

datapythonista left a comment

datapythonista Apr 30, 2018

datapythonista Apr 30, 2018

datapythonista Apr 30, 2018

datapythonista Apr 30, 2018

datapythonista Apr 30, 2018

datapythonista Apr 30, 2018

pep8speaks commented Apr 30, 2018 •

edited

Loading

alkamid commented Apr 30, 2018

datapythonista May 1, 2018

datapythonista May 1, 2018

alkamid commented May 2, 2018

datapythonista left a comment

DOC: Improve the docstring of String.str.zfill() #20864

DOC: Improve the docstring of String.str.zfill() #20864

Conversation

alkamid commented Apr 29, 2018

codecov bot commented Apr 29, 2018 • edited Loading

Codecov Report

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented Apr 30, 2018 • edited Loading

Comment last updated on July 07, 2018 at 14:20 Hours UTC

alkamid commented Apr 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alkamid commented May 2, 2018

datapythonista left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 29, 2018 •

edited

Loading

pep8speaks commented Apr 30, 2018 •

edited

Loading