-
Notifications
You must be signed in to change notification settings - Fork 143
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
Dryrun response #283
Dryrun response #283
Conversation
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.
Looks very useful.
I have a couple of suggestions to shorten the code, and some nit-picks that aren't as crucial.
A general question that we should bring up at standup as well: How do we feel about SDK's getting out of sync? I don't believe that this sort of helper functionality exists in any of the other SDK's, and is that ok? I think it should be ok. We may need to signal that this is special to python by having it live in a directory such as extras
or goodies
...
@tzaffi thanks for the review!
I expect to add it to the rest of them. I like using python as a testing ground for these to see if it makes sense and find a good pattern. |
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.
LGTM
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.
Looks like a nice improvement, though it would be great if there were at least one unit test.
The code seems very testable, since it does not rely on any REST API calls
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.
looks pretty neat to me (not sure if we want to ensure fields in DryrunTraceLine
or DryrunStackValue
), waiting on the test cases
Before merging, I want to raise a few more things that may be useful to include.
|
I changed the formatting a bit, to add line number, unquote stack values/convert to hex for bytes, and if 0 is passed for spaces use the longest line length. Curious to hear thoughts on this format instead
|
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
Co-authored-by: Zeph Grunschlag <[email protected]>
315d413
to
78aa402
Compare
5254423
to
8ded7ec
Compare
Yes, @barnjamin did this correctly in 8ded7ec |
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.
LGTM again
API should look something like:
and output