-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
bpo-29526 Add reference to help('FORMATTING') in format() builtin #166
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
aktech
commented
Feb 19, 2017
•
edited
Loading
edited
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 our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow these steps to rectify the issue:
Thanks again to your contribution and we look forward to looking at it! |
@@ -77,7 +77,9 @@ PyDoc_STRVAR(builtin_format__doc__, | |||
"\n" | |||
"Return value.__format__(format_spec)\n" | |||
"\n" | |||
"format_spec defaults to the empty string"); | |||
"format_spec defaults to the empty string.\n" |
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.
Python/clinic/bltinmodule.c.h
is generated file. Changes should be made in Python/bltinmodule.c
. Run make clinic
for regenerating Python/clinic/bltinmodule.c.h
.
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.
Oh, I see. Thanks, fixing now.
17428a5
to
2a96971
Compare
ping @serhiy-storchaka |
Python/bltinmodule.c
Outdated
@@ -578,12 +578,13 @@ format as builtin_format | |||
|
|||
Return value.__format__(format_spec) | |||
|
|||
format_spec defaults to the empty string | |||
format_spec defaults to the empty string. | |||
See help('FORMATTING') for description. |
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.
The initial section of help('FORMATTNG') describes replacement fields and everything but format specs. This could confuse a beginner. So I suggest as an alternative:
See the Format Specification Mini-Language section of help('FORMATTING').
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 agree with @terryjreedy that the pointer to the other docs isn't quite right. My suggestion would be to replace the second added sentence with:
See the Format Specification Mini-Language section of help('FORMATTING') for details.
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.
Hi @aktech, sorry for the long delay on the review here. Would you mind taking a look at the suggested amendments to the precise wording?
Python/bltinmodule.c
Outdated
@@ -578,12 +578,13 @@ format as builtin_format | |||
|
|||
Return value.__format__(format_spec) | |||
|
|||
format_spec defaults to the empty string | |||
format_spec defaults to the empty string. | |||
See help('FORMATTING') for description. |
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 agree with @terryjreedy that the pointer to the other docs isn't quite right. My suggestion would be to replace the second added sentence with:
See the Format Specification Mini-Language section of help('FORMATTING') for details.
Added the sprint label, as this PR was submitted at the PyCon Pune 2017 core development sprint. |
Codecov Report
@@ Coverage Diff @@
## master #166 +/- ##
==========================================
+ Coverage 83.41% 83.41% +<.01%
==========================================
Files 1367 1367
Lines 345516 345516
==========================================
+ Hits 288203 288211 +8
+ Misses 57313 57305 -8
Continue to review full report at Codecov.
|
@ncoghlan I have made the changes, please have a look. |
Merged, thanks! |
Sorry, @aktech and @ncoghlan, I could not cleanly backport this to |
GH-3491 is a backport of this pull request to the 3.6 branch. |
…tin (pythonGH-166). (cherry picked from commit 2e6bb44)
…tin (pythonGH-166). (cherry picked from commit 2e6bb44)
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were broken, because Stackless does not use them and didn't test them. This commit adds test code and makes the functions compatible with C-Python. It is now save to call them from extension modules, i.e. modules created by Cython.
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a potential reference counting problem.
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were broken, because Stackless does not use them and didn't test them. This commit adds test code and makes the functions compatible with C-Python. It is now save to call them from extension modules, i.e. modules created by Cython. (cherry picked from commit d377c06)
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a potential reference counting problem. (cherry picked from commit 187184b)
The C-API functions PyEval_EvalFrameEx() and PyEval_EvalFrame() were broken, because Stackless does not use them and didn't test them. This commit adds test code and makes the functions compatible with C-Python. It is now save to call them from extension modules, i.e. modules created by Cython. (cherry picked from commit d377c06)
Support using PyEval_EvalFrameEx() without a main tasklet. And fix a potential reference counting problem. (cherry picked from commit 187184b)