API: change default for sep in str.cat (in docstring) #23443

h-vetinari · 2018-11-01T08:51:38Z

In the course of #22725, @WillAyd mentioned (about the sep=None in Series.str.cat being immediately overwritten by sep = '' if sep is None else sep):

Not anything you need to change in this PR but if we just overwrite None here it would probably make more sense to change the default function signature to simply be ''

This changes the default accordingly. Not sure this even needs a deprecation cycle? It's a pretty trivial change, and I doubt there are people relying on sep=None explicitly.

pep8speaks · 2018-11-01T08:51:41Z

Hello @h-vetinari! Thanks for submitting the PR.

There are no PEP8 issues in the file pandas/core/strings.py !

codecov · 2018-11-01T09:34:06Z

Codecov Report

Merging #23443 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #23443   +/-   ##
=======================================
  Coverage   92.23%   92.23%           
=======================================
  Files         161      161           
  Lines       51197    51197           
=======================================
  Hits        47220    47220           
  Misses       3977     3977

Flag	Coverage Δ
#multiple	`90.61% <ø> (ø)`	⬆️
#single	`42.27% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/core/strings.py	`98.58% <ø> (ø)`	⬆️
pandas/core/indexes/datetimes.py	`96.41% <0%> (ø)`	⬆️
pandas/core/indexes/api.py	`99% <0%> (ø)`	⬆️
pandas/core/frame.py	`97.03% <0%> (ø)`	⬆️
pandas/core/generic.py	`96.81% <0%> (ø)`	⬆️
pandas/core/arrays/datetimes.py	`98.84% <0%> (ø)`	⬆️

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 d78bd7a...2d1bf05. Read the comment docs.

gfyoung · 2018-11-01T17:57:59Z

Given that we were setting sep to the empty string under-hood, surfacing it explicitly as the default seems reasonable and wouldn't require a deprecation cycle.

jschendel · 2018-11-02T03:01:56Z

pandas/core/strings.py

@@ -2074,8 +2074,9 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):

            If others is None, the method returns the concatenation of all
            strings in the calling Series/Index.
-        sep : string or None, default None
-            If None, concatenates without any separator.
+        sep : string, default ''


string --> str; iirc when specifying parameter types we want to use the Python function name (e.g. bool instead of boolean, dict instead of dictionary, etc.). Can you also modify na_rep a couple lines below?

jreback · 2018-11-02T13:43:04Z

pandas/core/strings.py

@@ -2196,8 +2197,6 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):

        if isinstance(others, compat.string_types):
            raise ValueError("Did you mean to supply a `sep` keyword?")
-        if sep is None:


I think just adding the doc-string update is enough. we commondly accept none, then actuall do this check here. The reason is if you actually pass None your change will fail (pls add a test to check that)

h-vetinari · 2018-11-02T15:28:41Z

@jreback

I think just adding the doc-string update is enough. we commondly accept none, then actuall do this check here. The reason is if you actually pass None your change will fail (pls add a test to check that)

How about the following - I went with re-adding the sep is None check, but emitting a warning. That will definitely not break anything, and emission of the warning should be very rare (as I believe hardly anyone will be passing sep=None directly).

jreback · 2018-11-03T14:12:02Z

pandas/core/strings.py

@@ -2197,6 +2198,10 @@ def cat(self, others=None, sep=None, na_rep=None, join=None):
        if isinstance(others, compat.string_types):
            raise ValueError("Did you mean to supply a `sep` keyword?")
        if sep is None:
+            warnings.warn('Passing `sep=None` to .str.cat is deprecated and '


pls remove this and just update as I suggested.

jreback · 2018-11-04T15:52:09Z

pandas/core/strings.py

@@ -2051,7 +2051,7 @@ def _get_series_list(self, others, ignore_index=False):
                return ([Series(others, index=idx)], False)
        raise TypeError(err_msg)

-    def cat(self, others=None, sep=None, na_rep=None, join=None):
+    def cat(self, others=None, sep='', na_rep=None, join=None):


there is likely some other check for sep is None in the codebase, this shouldn't pass, and as I said pls change back to the original modulo the doc-string.

h-vetinari · 2018-11-05T18:33:54Z

@jreback

there is likely some other check for sep is None in the codebase, this shouldn't pass, and as I said pls change back to the original modulo the doc-string.

What check for sep is None do you imagine there would be? It's not in the code of str.cat itself, but I guess I don't understand what you mean.

jreback · 2018-11-06T03:55:35Z

thanks!

API: change default for sep in str.cat

5e2bd40

gfyoung added API Design Strings String extension data type and string data labels Nov 1, 2018

gfyoung requested review from jreback and WillAyd and removed request for jreback November 1, 2018 17:58

jschendel reviewed Nov 2, 2018

View reviewed changes

jreback reviewed Nov 2, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into str_cat_sep

8f595e0

h-vetinari added 2 commits November 2, 2018 16:31

Review (jschendel & jreback)

d361dce

Retrigger Circle

af66b94

jreback reviewed Nov 3, 2018

View reviewed changes

h-vetinari added 4 commits November 3, 2018 16:40

Merge remote-tracking branch 'upstream/master' into str_cat_sep

f39416e

Review (jreback)

ae638b9

Retrigger CircleCI

712642b

Retrigger CircleCI

29be975

jreback requested changes Nov 4, 2018

View reviewed changes

Review (jreback)

0f32510

h-vetinari changed the title ~~API: change default for sep in str.cat~~ API: change default for sep in str.cat (in docstring) Nov 5, 2018

h-vetinari added 2 commits November 5, 2018 01:17

Retrigger CircleCI

608f2d2

Retrigger Circle

2d1bf05

jreback added this to the 0.24.0 milestone Nov 6, 2018

jreback approved these changes Nov 6, 2018

View reviewed changes

jreback merged commit c6366f5 into pandas-dev:master Nov 6, 2018

h-vetinari deleted the str_cat_sep branch November 6, 2018 10:13

JustinZhengBC pushed a commit to JustinZhengBC/pandas that referenced this pull request Nov 14, 2018

API: change default for sep in str.cat (in docstring) (pandas-dev#23443)

9436e21

tm9k1 pushed a commit to tm9k1/pandas that referenced this pull request Nov 19, 2018

API: change default for sep in str.cat (in docstring) (pandas-dev#23443)

c6ea694

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

API: change default for sep in str.cat (in docstring) (pandas-dev#23443)

a6e8a68

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

API: change default for sep in str.cat (in docstring) (pandas-dev#23443)

5818916

h-vetinari mentioned this pull request Jul 26, 2019

DEPR: execute deprecations for str.cat in v1.0 #27611

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: change default for sep in str.cat (in docstring) #23443

API: change default for sep in str.cat (in docstring) #23443

h-vetinari commented Nov 1, 2018

pep8speaks commented Nov 1, 2018

codecov bot commented Nov 1, 2018 •

edited

Loading

gfyoung commented Nov 1, 2018

jschendel Nov 2, 2018

jreback Nov 2, 2018

h-vetinari commented Nov 2, 2018

jreback Nov 3, 2018

jreback Nov 4, 2018

h-vetinari commented Nov 5, 2018

jreback commented Nov 6, 2018

API: change default for sep in str.cat (in docstring) #23443

API: change default for sep in str.cat (in docstring) #23443

Conversation

h-vetinari commented Nov 1, 2018

pep8speaks commented Nov 1, 2018

codecov bot commented Nov 1, 2018 • edited Loading

Codecov Report

gfyoung commented Nov 1, 2018

jschendel Nov 2, 2018

Choose a reason for hiding this comment

jreback Nov 2, 2018

Choose a reason for hiding this comment

h-vetinari commented Nov 2, 2018

jreback Nov 3, 2018

Choose a reason for hiding this comment

jreback Nov 4, 2018

Choose a reason for hiding this comment

h-vetinari commented Nov 5, 2018

jreback commented Nov 6, 2018

codecov bot commented Nov 1, 2018 •

edited

Loading