-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-37937: Mention frame.f_trace in sys.settrace docs #15439
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
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. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
@brandtbucher Do you think this PR qualifies for the "skip issue" and "skip news" labels? If it's no on the latter I can add a news entry, let me know. |
I think there's still some confusion. |
Point taken, changed the text to remove the reference to optimization.
…On Sat, Aug 24, 2019 at 8:56 AM Alex Hall ***@***.***> wrote:
I think there's still some confusion. f_trace is not being used as an
optimisation in PySnooper. It's there for correctness, not performance.
Specifically it makes with snoop(): actually trace the frame that it's
in, since sys.settrace() can't. Using it as an optimisation instead of
just sys.settrace() would be very difficult because you generally want to
start tracing frames from the beginning of their execution but it's hard to
obtain a frame anywhere but from within it. Are you using this in a project
of your own like that?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#15439?email_source=notifications&email_token=AAAN3SV2ZFUJFXOTBSQHHW3QGDESJA5CNFSM4IPCXPX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5BZOSY#issuecomment-524523339>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAN3SWTKBGRXXYMP4OQQETQGDESJANCNFSM4IPCXPXQ>
.
|
I think it's important to mention (though I'm not sure of the best way to word it) that if you implement a debugger and you provide some function to 'start tracing here' like a breakpoint (e.g. |
Here's a suggestion:
|
@cool-RR Given that this isn’t a trivial doc fix, I’d still include a basic NEWS entry. |
Added to NEWS, created a bpo ticket. Let me know whether anything else is missing. |
With help from 2 non-coredev reviews, this looks ready for a coredev to review, and if the addition is correct, likely merge. This is too far from my areas of expertise to know. If the addition applies to 3.8 and 3.7, add backport labels. I suggest that one of you peruse the experts list and select a couple to request. |
I'd like to add the backport label for 3.7 and 3.8, but I'm not seeing a way to do that. Can anyone help? |
@benjaminp @pitrou I'm tagging you guys because I saw you touched |
I guess @benjaminp and @pitrou are busy. Maybe @ncoghlan or @serhiy-storchaka would like to review this? |
Paging @abalkin , maybe you'd like to review this? |
Belongs to only in the sense of "I think I'm the one that touched it last" ;) The |
Thanks for chiming in, Nick. Are you saying that because of your statement above, you disagree with this addition to the docs? |
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.
This mostly looks excellent, thanks - just one point that I think could be confusing if we don't adjust the wording slightly.
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 |
I have made the requested changes; please review again |
Thanks for making the requested changes! @ncoghlan: please review the changes made to this pull request. |
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.
Thanks! There was one further amendment needed (related to exactly what's required to enable the C level trace hook that calls frame.f_trace), which I went ahead and made after double-checking the exact requirements in the interactive REPL.
GH-16296 is a backport of this pull request to the 3.8 branch. |
GH-16297 is a backport of this pull request to the 3.7 branch. |
Mention frame.f_trace in sys.settrace docs, as well as the fact you still need to call `sys.settrace` to enable the tracing machinery before setting `frame.f_trace` will have any effect. (cherry picked from commit 9c2682e) Co-authored-by: Ram Rachum <ram@rachum.com>
Mention frame.f_trace in sys.settrace docs, as well as the fact you still need to call `sys.settrace` to enable the tracing machinery before setting `frame.f_trace` will have any effect. (cherry picked from commit 9c2682e) Co-authored-by: Ram Rachum <ram@rachum.com>
Wheee 😊 And thanks to @alexmojaki for the guidance. |
Mention frame.f_trace in sys.settrace docs, as well as the fact you still need to call `sys.settrace` to enable the tracing machinery before setting `frame.f_trace` will have any effect. (cherry picked from commit 9c2682e) Co-authored-by: Ram Rachum <ram@rachum.com>
After I released
PySnooper
, @alexmojaki added the awesome optimization of usingf_trace
instead ofsys.settrace
. He needed to explain to me how it works, because I couldn't find any documentation off_trace
online. So I figured I'd add it to Python's documentation.https://bugs.python.org/issue37937