-
-
Notifications
You must be signed in to change notification settings - Fork 18k
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
API: Deprecate regex=True default in Series.str.replace #36695
Merged
Merged
Changes from 39 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
52f6d78
API: Deprecate regex=True default in Series.str.replace
dsaxton 3b9bcb2
Format
dsaxton c51e837
Stacklevel
dsaxton 3761625
Test
dsaxton 7583b7f
Test name
dsaxton 8e0d1a5
Link discussion
dsaxton 79e4400
Set regex
dsaxton ecc5786
Update
dsaxton cf659b0
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 628cfa3
Update
dsaxton f216022
Fix
dsaxton e6c79a3
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton e6b3e0f
Check for str
dsaxton e3ce080
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 752d322
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton ee67284
Doc updates
dsaxton c832a21
Nit
dsaxton 09b07e6
Silence some warnings
dsaxton c8541da
Edit
dsaxton bd31857
Edit
dsaxton 377a2ba
Question mark
dsaxton d6b4fbe
Update doc
dsaxton 84dfe71
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 5cfbc04
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 1b53051
Add back
dsaxton 1931c31
Oops
dsaxton 20bfe16
Note
dsaxton 6ff5955
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton cea44a7
Fix
dsaxton 4376bca
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 482d5c3
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton cd18347
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton f872011
Update warning
dsaxton c0a473e
rst lint
dsaxton 727986e
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton f49f778
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton d3d155a
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 89013db
Nit + emphasis
dsaxton e78017c
regex=False
dsaxton 0150b2b
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton e396ce5
Update
dsaxton e799b12
fix
dsaxton 9f7545f
rst fix
dsaxton 6be90a4
Merge remote-tracking branch 'upstream/master' into replace-regex-def…
dsaxton 8a4a833
Make a warning
dsaxton File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1170,7 +1170,7 @@ def fullmatch(self, pat, case=True, flags=0, na=None): | |
return self._wrap_result(result, fill_value=na, returns_string=False) | ||
|
||
@forbid_nonstring_types(["bytes"]) | ||
def replace(self, pat, repl, n=-1, case=None, flags=0, regex=True): | ||
def replace(self, pat, repl, n=-1, case=None, flags=0, regex=None): | ||
r""" | ||
Replace each occurrence of pattern/regex in the Series/Index. | ||
|
||
|
@@ -1288,6 +1288,20 @@ def replace(self, pat, repl, n=-1, case=None, flags=0, regex=True): | |
2 NaN | ||
dtype: object | ||
""" | ||
if regex is None: | ||
if isinstance(pat, str) and any(c in pat for c in ".+*|^$?[](){}\\"): | ||
# warn only in cases where regex behavior would differ from literal | ||
msg = ( | ||
"The default value of regex will change from True to False " | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an you make sure that we are testing both of these warnings (as i see you only added a single test) |
||
"in a future version." | ||
) | ||
if len(pat) == 1: | ||
msg += ( | ||
" In addition, single character regular expressions will" | ||
"*not* be treated as literal strings when regex=True." | ||
) | ||
warnings.warn(msg, FutureWarning, stacklevel=3) | ||
regex = True | ||
result = self._array._str_replace( | ||
pat, repl, n=n, case=case, flags=flags, regex=regex | ||
) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -449,3 +449,9 @@ def test_replace_with_compiled_regex(self): | |
result = s.replace({regex: "z"}, regex=True) | ||
expected = pd.Series(["z", "b", "c"]) | ||
tm.assert_series_equal(result, expected) | ||
|
||
def test_str_replace_regex_default_raises_warning(self): | ||
# https://github.com/pandas-dev/pandas/pull/24809 | ||
s = pd.Series(["a", "b", "c"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you check the messages on this warning |
||
with tm.assert_produces_warning(FutureWarning, check_stacklevel=False): | ||
s.str.replace("^.$", "") |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can make this a note / warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also add a versionchanged tag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a versionchanged tag. Do we also need one in the docstring?
I'm not sure what's meant by note / warning since I've added a whatsnew note and warning. Do you mean put the text itself in the whatsnew?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no i mean a
..note::
a sphinx-note (which puts a highlite box around this)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could also do a
..warning::
which is a red-box and more prominent, but either way want to call out thisThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this a .. warning