-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
ENH: support str translate for StringMethods #10052
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
I don't know if I find it a good idea to have different behaviour for python 2 vs 3. Why not just going with the python 3 signature? |
the thing is it's more than the signature that changed from python 2 to 3. The meaning of |
Correct, but I think single internal func is better for API doc on the website to show both python 2 and 3 info. How about:
|
If the change between python 2 and 3 is that big, I think even more reason for us to choose one behaviour (and thus, python 3 behaviour is of course the most logical). Users of python 2 will eventually have to change their code for python 3, so if they start using the pandas string method, they can already adapt (and otherwise they can always use the apply approach). We should follow the behaviour of the standard string methods as much as possible, but here I would personally deviate. Do you know if there is a reason the behaviour changed? Did the functionality goes elwewhere? |
so first of all, don't get me wrong - I think the python 3 signature is better and I'm a big advocate for using python 3. It's just that sticking with the python 3 signature in our implementation means that python 2 users will lose some functionality. And I'm open to making that tradeoff. @jorisvandenbossche just to be clear, no functionality of the standard |
so, python 2 users won't really lose some functionality, but just have to adapt their code somewhat? I also understand the reasons to go for python 2/3 dependent behaviour, but as this is a 'new' feature for pandas, I would lean towards making a choice for 'one way', as we have the choice to do that |
@jorisvandenbossche if we make the signature |
Ah, sorry, I didn't really look in detail to the |
I think the suggestion of @sinhrks is good then, having one docstring that has information for both python 2 and 3 (as we don't have separate docs) |
I haven't used |
I agree here @sinhrks seems reasonable. |
Sounds good thank you all for the feedback! Will update shortly. |
updated, please take a look |
def str_translate(arr, table, deletechars=None): | ||
""" | ||
Map all characters in the string through the given mapping table. | ||
Equivalent to standard ``str.translate``. Note that the optional argument |
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.
pls use ":meth:
str.translate" (and "`:func:`string.maketrans
" for another) like other docstring.
40be2fc
to
d01725c
Compare
Can you update the docstring here as well? |
@jorisvandenbossche do you mean the release note? |
yes, of course .. :-) |
yes definitely :) the only thing is that this would be on the same line as the PR for |
@mortada this was passing before yes? and you just changed docs right? |
yes this was passing before |
@mortada ok, this needs a rebase now that I merged the other. pls ping when ready. |
ok cool i'll rebase |
ok rebase is done, updated |
ENH: support str translate for StringMethods
@mortada ok thank you! |
sure thanks a lot! |
as a part of #9111
note that this handles both py2 and py3 as the signature for
str.translate
has changed from py2 to py3@sinhrks @jreback