Repository navigation
Conversation
Collaborator
Coverage Report for CI Build 39895159561Coverage remained the same at 91.587%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Contributor
|
Hi @chiually, I find the PR description quite long. Please rewrite it in your own words and shorten it. Focus on a high-level description and a concrete example. I don't need implementation details in the PR description; I will infer this by reviewing the code directly. Only point out anything which you believe is surprising. |
david-yz-liu
left a comment
Contributor
There was a problem hiding this comment.
@chiually Nice work, I left a few inline comments about the overall approach, but I think this is definitely on the right track. Please add test cases and update the documentation.
Contributor
Author
|
I implemented your feedback and added two additional changes:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Currently, the
snapshotfunction is called with the import call and a code addition for the function call. Instead, we want to be able to write the call tosnapshotas a "magic comment" (ex.# pyta: snapshot()) that would be ignored when run normally but take the snapshot of memory when its file is run through the newrun_with_snapshot_commentsfunction.Example
Screenshots of your changes (if applicable)
Type of Change
(Write an
Xor a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]into a[x]in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
Some edits needed to be made to
snapshot.pyto stop thesnapshotfunction from capturing the frame that callsrun_with_snapshot_comments.Currently, the magic comment only works for comments on its own line (as pictured in the example). Let me know what you think of the current implementation and I will add for inline comments as well (ex.
x = 5 # hello)Also when inserting the snapshot call, a standalone comment's indentation decides how deeply nested it is, and its line number decides which branch it goes in (ex.
ifor theelse). Theastmodule records whereif,exceptandcaselines are but not whereelse:andfinally:lines are, so for this case we take the first line of code after the previous branch ends to be theelse:orfinally:line.