ENH: Add StringMethod.find and rfind by sinhrks · Pull Request #9386 · pandas-dev/pandas (original) (raw)

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 }})

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).

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 versionadded directives (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 Notes is duplicated with the "Method Summary" in text.rst. How about adding Notes in each docstring, and change "Method Summary" section to a simple link to "String handling" section in api.rst.

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 string methods (not only find and rfind).

This is one thing, and added a description for this PR.

jreback added a commit that referenced this pull request

Feb 16, 2015

ENH: Add StringMethod.find and rfind

Labels