ENH: Add StringMethod.find and rfind by sinhrks · Pull Request #9386 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits1 Checks0 Files changed
Conversation
This file contains hidden or 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
[ Show hidden characters]({{ revealButtonHref }})
| end : int |
|---|
| Right edge index |
| side : {'left', 'right'}, default 'left' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would document this as switching between find (left side) and rfind (right side).
| def str_find(arr, sub, start=0, end=None, side='left'): |
|---|
| """ |
| Return indexes in each strings where the substring is |
| wholly contained between [start:end]. Return -1 on failure. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'wholly' sounds a bit odd to me (non-native speaker), is 'fully' better?
@sinhrks I added some documentation comments. Further:
- I think you may group all the StringMethods enhancement in a section under 'new features' (as it starts to become a large list)
- Maybe we should start adding
versionaddeddirectives (see here) to newly added functions? We don't do this consistently right now, but I think we should do that more.
and another:
- maybe add a 'Notes' section saying it is equivalent to the standard library's
str.find(I even think you can link to that using:func:str.find``
Modified once.
- About release note, let me do once finish StringMethods should have the same methods as standard str #9111 (added as AI).
- Adding
Notesis duplicated with the "Method Summary" intext.rst. How about addingNotesin each docstring, and change "Method Summary" section to a simple link to "String handling" section inapi.rst.
| """) |
|---|
| @Appender(_shared_docs['find'] % dict(side='lowest', |
| also='rfind : Return highest indexes in each strings')) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think rfind has to be Series.str.rfind (and same below)
And on the note, I don't think that is a problem that is would be duplicated (that is the point of having both docstrings and narrative docs, there will always be some duplication).
If the table in text.rst can be replaced by an autosummary table is separate issue I think. Positive side is that it would add the explanation manually, but at the moment I like more how the table looks in text.rst (better overview) than how it is on the api pages.
It is maybe not necessary to add it in a Note, just adding 'equivalent to standard library's str.find' in the explanation in the beginning of the docstring is also good.
Maybe my word choice is inappropriate. What I'm caring is uniformity of styles and descriptions. What I expect is all the docstring should have similar format, and sufficient information in a single place.
In this respect, current doc should be revised in separate PR to cover:
- All the docstring should provide "better overview" and this should be used both in "text.rst" and "api.rst".
- Add descriptions for standard
stringmethods (not onlyfindandrfind).
This is one thing, and added a description for this PR.
jreback added a commit that referenced this pull request
ENH: Add StringMethod.find and rfind
