-
-
Notifications
You must be signed in to change notification settings - Fork 31.5k
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
Indicate that abs() method accept argument that implement __abs__(), … #7783
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. When your account is ready, please add a comment in this pull request Thanks again for your contribution, we look forward to reviewing it! |
I signed the CLA, Thank you for reviewing. |
This comment has been minimized.
This comment has been minimized.
@eric-wieser Thanks, I fixed it now. |
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 think the wording could be improved: what does "The argument may be an integer or a floating point number." really mean now? You should explain that this applies to integers, floating point numbers, complex numbers and anything defining __abs__
.
Doc/library/functions.rst
Outdated
@@ -43,8 +43,8 @@ are always available. They are listed here in alphabetical order. | |||
.. function:: abs(x) | |||
|
|||
Return the absolute value of a number. The argument may be an | |||
integer or a floating point number. If the argument is a complex number, its | |||
magnitude is returned. | |||
integer or a floating point number, If *x* defines :meth:`__abs__`, |
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.
There's a capital letter after a coma.
Also I have another proposition about this coma:
[...] integer, a floating point number, a complex number, or any object defining the :meth:
__abs__
method. If the argument is a complex number, its magnitude is returned.
I agree with @jdemeyer, it could be "simplified" even to Accept any object *x* implementing :meth:`__abs__`
, but this documentation is not the language reference, its audience is newcomers, they care about being able to pass the three builtins they just learnt int
, float
, complex
like abs(-5)
and abs(-5.5)
.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
@Windsooon, please resolve the merge conflict and address the code review comments. Thank you! |
This comment has been minimized.
This comment has been minimized.
To make it easier, how about
|
This comment has been minimized.
This comment has been minimized.
Sorry for the mistake. I just fixed it. |
Yes, by using
instead of a raw abs. I'm not a native english speaker but I think the first or is now redundent, what about:
I'd maybe prefer a bit
|
@JulienPalard Thank you. I updated the PR based on your suggestion. |
@Windsooon could you resolve the conflict? |
rebase commonly messes up branches and PRs with the cpython workflow. Did you do git merge in master before updating the branch? git merge upstream/master usually works. There might be a merge conflict, but not likely with a narrow PR like this one. You could still try merge on your local branch if not deleted yet. |
@terryjreedy Thank you. I just delete the old branch. However, I don't know why my PR will conflict with the current master branch. I will check it out later. |
…just like call() method in the docs