-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
DOC: Add regex example in str.split docstring #26267
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
Conversation
a4c67a3
to
c6eac52
Compare
Update: Fixed the PEP8 issues with the force-pushed commits. |
pandas/core/strings.py
Outdated
|
||
>>> s = pd.Series(["1+1=2", np.nan]) | ||
|
||
>>> s.str.split("\\+|=", expand=True) |
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.
Could you use a raw string here instead? That would be more idiomatic to Python
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.
>>> s.str.split("\\+|=", expand=True) | |
>>> s.str.split(r"\+|=", expand=True) |
I tried doing it this way but PyCharm's PEP8 checker still detects the \+
as an invalid escape sequence. Should I just try force-pushing and checking if it will pass the pep8speaks
check?
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.
Update: I've force-pushed an edit that applies the changes I've outlined above.
@WillAyd pep8speaks
threw out a W605 complaining about the invalid escape sequence. Do I keep these changes or do I revert it back to the "\\+|="
string?
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.
Hmm OK that's strange. Can you check the code base for how we've handled elsewhere? I'd be surprised if this is the first time we've seen 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.
I turned the entire docstring into a raw string by adding an r
before the triple quotation marks at the beginning. This removed the PEP8 error both in my local machine and from pep8speaks
. Kindly review. Thanks!
Codecov Report
@@ Coverage Diff @@
## master #26267 +/- ##
==========================================
- Coverage 91.98% 91.97% -0.01%
==========================================
Files 175 175
Lines 52386 52386
==========================================
- Hits 48186 48182 -4
- Misses 4200 4204 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #26267 +/- ##
==========================================
- Coverage 91.99% 91.98% -0.01%
==========================================
Files 175 175
Lines 52379 52379
==========================================
- Hits 48184 48180 -4
- Misses 4195 4199 +4
Continue to review full report at Codecov.
|
f07c0a7
to
a0e3d8c
Compare
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.
Minor nit but otherwise lgtm. @gfyoung care to take a look and merge if satisfied?
a0e3d8c
to
ceb29a8
Compare
Thanks @vandenn ! |
git diff upstream/master -u -- "*.py" | flake8 --diff
Adding the regex example in the
str.split()
documentation to make people aware of the need to escape special characters when using regular expressions as the pattern.Error from docstring validation already exists in master's HEAD with no modifications. No additional error was introduced by the new docstring content.
Output of docstring validation: