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: specify regex=True in str.replace #41397

Merged
merged 5 commits into from
May 10, 2021

Conversation

rhshadrach
Copy link
Member

  • Ensure all linting tests pass, see here for how to run them

ref #36695

When passing a callable repl, str.replace will raise if regex is False. So it seems to me that in the future, when the default is changed, we should be ignoring the regex argument completely (and update the documentation to that effect). This is slightly magical, but I think better than having a default that raises.

If this is the correct way forward, then we don't need to warn when repl is callable.

cc @dsaxton @jorisvandenbossche

@rhshadrach rhshadrach added Strings String extension data type and string data Deprecate Functionality to remove in pandas Warnings Warnings that appear or should be added to pandas labels May 9, 2021
@dsaxton
Copy link
Member

dsaxton commented May 9, 2021

@rhshadrach If I'm understanding correctly, I think we would not want to skip this warning when a callable is passed, because that behavior will go from not raising to raising in the future. That way users who are implicitly relying on this won't be surprised when suddenly their code starts failing. It almost feels like the warning message could even be more verbose in this case.

Not sure if we'd want to update this here too, but I just noticed the docstring for this method is grammatically a bit odd:

Determines if assumes the passed-in pattern is a regular expression:

@rhshadrach
Copy link
Member Author

Thanks @dsaxton - I think that would not be the case if we followed my suggested way forward in the OP (and comment in the code, but perhaps that comment is too terse). Thoughts on this approach, where we would be ignoring the regex keyword when repl is callable?

Not sure if we'd want to update this here too, but I just noticed the docstring for this method is grammatically a bit odd

I think this would be okay to address here, will do so.

@dsaxton
Copy link
Member

dsaxton commented May 9, 2021

I think that would not be the case if we followed my suggested way forward in the OP (and comment in the code, but perhaps that comment is too terse). Thoughts on this approach, where we would be ignoring the regex keyword when repl is callable?

Ah, I misunderstood. Personally I like the idea of being consistent in how regex is handled and breaking loudly if a callable is passed along with False for regex (either implicitly or not). I think it'd make the method easier to reason about, and forces end users to be explicit about what they want to do.

@rhshadrach rhshadrach changed the title DEPR: Don't warn in str.replace when repl is callable DOC: specify regex=True in str.replace May 9, 2021
@rhshadrach rhshadrach added Docs and removed Warnings Warnings that appear or should be added to pandas labels May 9, 2021
@rhshadrach
Copy link
Member Author

Makes sense @dsaxton - updated this PR to only fix documentation.

Copy link
Member

@dsaxton dsaxton left a comment

Choose a reason for hiding this comment

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

Thanks @rhshadrach, looks like an improvement to me

@jreback jreback added this to the 1.3 milestone May 10, 2021
@jreback jreback merged commit 8159e8d into pandas-dev:master May 10, 2021
@jreback
Copy link
Contributor

jreback commented May 10, 2021

yep this lgtm. thanks @rhshadrach

@rhshadrach rhshadrach deleted the str_replace_depr_callable branch May 10, 2021 22:12
JulianWgs pushed a commit to JulianWgs/pandas that referenced this pull request Jul 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas Docs Strings String extension data type and string data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants