| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
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>
.
|
Sorry, something went wrong.
|
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. pdb.set_trace or pysnooper.Tracer.__enter__), then you need to set f_trace on the frame where that function was called. sys.settrace() will never do that on its own because it only starts working when you enter a new scope. That is the thing that confused and bothered me when I was implementing the functionality in both PySnooper and another project of mine, and I had to look at pdb's source to figure out what was going on. |
Sorry, something went wrong.
|
Here's a suggestion:
|
Sorry, something went wrong.
|
@cool-RR Given that this isn’t a trivial doc fix, I’d still include a basic NEWS entry. |
Sorry, something went wrong.
|
Added to NEWS, created a bpo ticket. Let me know whether anything else is missing. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
|
@benjaminp @pitrou I'm tagging you guys because I saw you touched f_trace in the past. Would any of you care to review this PR? |
Sorry, something went wrong.
|
I guess @benjaminp and @pitrou are busy. Maybe @ncoghlan or @serhiy-storchaka would like to review this? |
Sorry, something went wrong.
|
Paging @abalkin , maybe you'd like to review this? |
Sorry, something went wrong.
|
Belongs to only in the sense of "I think I'm the one that touched it last" ;) The f_trace frame attribute is certainly intended to be read/write, it's just that the conventional way of setting it is indirectly via the return value from a globally installed trace function. |
Sorry, something went wrong.
|
Thanks for chiming in, Nick. Are you saying that because of your statement above, you disagree with this addition to the docs? |
Sorry, something went wrong.
There was a problem hiding this comment.
This mostly looks excellent, thanks - just one point that I think could be confusing if we don't adjust the wording slightly.
Sorry, something went wrong.
|
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. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request. |
Sorry, something went wrong.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
|
Thanks for making the requested changes! @ncoghlan: please review the changes made to this pull request. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
GH-16296 is a backport of this pull request to the 3.8 branch. |
Sorry, something went wrong.
|
GH-16297 is a backport of this pull request to the 3.7 branch. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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>
| Back | FazBrowse Home | New Git URL |
After I released PySnooper, @alexmojaki added the awesome optimization of using f_trace instead of sys.settrace. He needed to explain to me how it works, because I couldn't find any documentation of f_trace online. So I figured I'd add it to Python's documentation.
https://bugs.python.org/issue37937