str.replace('.','') should replace every character? (fix)
State: closed

- [ X] closes #24804
- [X ] tests added / passed
- [ X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [ X] fix replace pattern problem



Labels:
name: Strings
description: String extension data type and string data

Comments:
Hello @alarangeiras! Thanks for updating the PR.

 - In the file [`pandas/core/strings.py`](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py), following are the PEP8 issues :

> [Line 580:17](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L580): [E225](https://duckduckgo.com/?q=pep8%20E225) missing whitespace around operator
> [Line 580:17](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L580): [E711](https://duckduckgo.com/?q=pep8%20E711) comparison to None should be 'if cond is None:'
> [Line 581:80](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L581): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (92 > 79 characters)
> [Line 581:93](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L581): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace
> [Line 582:29](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L582): [E127](https://duckduckgo.com/?q=pep8%20E127) continuation line over-indented for visual indent
> [Line 582:80](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/core/strings.py#L582): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (98 > 79 characters)

 - In the file [`pandas/tests/test_strings.py`](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/tests/test_strings.py), following are the PEP8 issues :

> [Line 1024:49](https://github.com/pandas-dev/pandas/blob/93a0715e36a4e292fcf9389d581646d5ffab5ec1/pandas/tests/test_strings.py#L1024): [W291](https://duckduckgo.com/?q=pep8%20W291) trailing whitespace



##### Comment last updated on January 17, 2019 at 17:34 Hours UTC
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=h1) Report
> Merging [#24809](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/17a6bc56e5ab6ad3dab12d3a8b20ed69a5830b6f?src=pr&el=desc) will **decrease** coverage by `49.46%`.
> The diff coverage is `0%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24809/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=tree)

```diff
@@             Coverage Diff             @@
##           master   #24809       +/-   ##
===========================================
- Coverage   92.38%   42.92%   -49.47%     
===========================================
  Files         166      166               
  Lines       52382    52382               
===========================================
- Hits        48395    22485    -25910     
- Misses       3987    29897    +25910
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `?` | |
| #single | `42.92% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `33% <0%> (-65.59%)` | :arrow_down: |
| [pandas/io/formats/latex.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvbGF0ZXgucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/core/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvY2F0ZWdvcmljYWwucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/sas/sas\_constants.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfY29uc3RhbnRzLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/plotting.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvcGxvdHRpbmcucHk=) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/tseries/converter.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL3RzZXJpZXMvY29udmVydGVyLnB5) | `0% <0%> (-100%)` | :arrow_down: |
| [pandas/io/formats/html.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2Zvcm1hdHMvaHRtbC5weQ==) | `0% <0%> (-99.35%)` | :arrow_down: |
| [pandas/core/groupby/categorical.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZ3JvdXBieS9jYXRlZ29yaWNhbC5weQ==) | `0% <0%> (-95.46%)` | :arrow_down: |
| [pandas/io/sas/sas7bdat.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXM3YmRhdC5weQ==) | `0% <0%> (-91.17%)` | :arrow_down: |
| [pandas/io/sas/sas\_xport.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL3Nhcy9zYXNfeHBvcnQucHk=) | `0% <0%> (-90.15%)` | :arrow_down: |
| ... and [124 more](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree-more) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=footer). Last update [17a6bc5...340ae89](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=h1) Report
> Merging [#24809](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/17a6bc56e5ab6ad3dab12d3a8b20ed69a5830b6f?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/24809/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #24809      +/-   ##
==========================================
- Coverage   92.38%   92.38%   -0.01%     
==========================================
  Files         166      166              
  Lines       52382    52382              
==========================================
- Hits        48395    48394       -1     
- Misses       3987     3988       +1
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.8% <100%> (-0.01%)` | :arrow_down: |
| #single | `42.92% <0%> (ø)` | :arrow_up: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/strings.py](https://codecov.io/gh/pandas-dev/pandas/pull/24809/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc3RyaW5ncy5weQ==) | `98.44% <100%> (-0.15%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=footer). Last update [17a6bc5...93a0715](https://codecov.io/gh/pandas-dev/pandas/pull/24809?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

I think now it's ok.
@WillAyd, is there a consensus about how document this issue?
Ah I see your point now - you’d essentially be doing that on top of the change here. I was assuming we would hold off on this change in lieu of the warning. 


Sent from my iPhone

> On Jan 17, 2019, at 10:42 AM, Tom Augspurger <notifications@github.com> wrote:
> 
> @TomAugspurger commented on this pull request.
> 
> In pandas/tests/test_strings.py:
> 
> > @@ -2924,7 +2932,7 @@ def test_pipe_failures(self):
>  
>          tm.assert_series_equal(result, exp)
>  
> -        result = s.str.replace('|', ' ')
> +        result = s.str.replace('|', ' ', regex=False)
> Why not just warn when the length of the pattern is 1 and regex=True?
> 
> Then I think there would be no way to have
> 
> In [5]: pd.Series(['a.c']).str.replace('.', 'b')
> # Warning: Interpreting '.' as a literal, not a regex...
> Out[5]:
> 0    abc
> dtype: object
> # no warning
> In [5]: pd.Series(['a.c']).str.replace('.', 'b', regex=True)
> Out[5]:
> 0    bbb
> dtype: object
> unless I"m missing something.
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or mute the thread.

No. Just ones as potentially disruptive as this.

On Thu, Jan 17, 2019 at 9:57 AM Allan Larangeiras <notifications@github.com>
wrote:

> *@alarangeiras* commented on this pull request.
> ------------------------------
>
> In pandas/tests/test_strings.py
> <https://github.com/pandas-dev/pandas/pull/24809#discussion_r248729082>:
>
> > @@ -2924,7 +2932,7 @@ def test_pipe_failures(self):
>
>          tm.assert_series_equal(result, exp)
>
> -        result = s.str.replace('|', ' ')
> +        result = s.str.replace('|', ' ', regex=False)
>
> Following this line of reasoning, from what I understand, every bug found
> should issue a warning of future adjustment?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/24809#discussion_r248729082>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ABQHIslncrVV0b7yBOox-uSCtXW_OyW7ks5vEJ2AgaJpZM4aEAxN>
> .
>

Sorry, I don't agree with this solution. I think it's the best if someone else make this fix.
Just noting the potential for this confusion was brought up when we added the `regex` parameter, though it didn't generate much discussion: https://github.com/pandas-dev/pandas/issues/16808#issuecomment-362670420

At the time I noted that changing this behavior would break back-compat (since the undocumented behavior that had been there since the beginning was literal replacement for 1-character strings and regex replacement for >1 character strings).

I'm totally on board with changing either the documentation or the behavior to be more consistent, but it definitely needs a deprecation cycle as suggested by @TomAugspurger. The behavior of .str.replace('.', '') without regex specified to replace periods, rather than every character, has been constant since at least <=0.16.
Thanks for the context @Liam3851, that's valuable.

@alarangeiras, does that make sense? Or are you done working on this?