-
Notifications
You must be signed in to change notification settings - Fork 132
Expand BlackboxPyTealer in an attempt to simplify dryrun request generation #335
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
|
Have you looked at functools.partial ? |
tzaffi
left a comment
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 is clearly an improvement, thank you! Since you're touching this code I also recommend you consider the following:
- the minor python 3.10 typing conventions I suggest
- I would be supportive of a clever use of
functools.partials. I'm curious how this would look like. - Now that
BlackboxPyTealercan also run dry-runs, I think the suffixPyTealeris somewhat misleading. Consider a better? name such asSubroutineRunnerorBlacboxTesteror you can probably come up with something better. (make sure to announce it on slack's #christening channel)
|
As of 7f5f526, I've made these changes per PR feedback:
Note - The build issue stems from the parent PR. I'll update the PR once the parent branch issue is resolved. |
Optionally proposes a change to #322 that may simplify blackbox test setup.
While reviewing #322, it seemed to me that it's common to heavily reference
BlackboxPyTealerwhen constructing a Graviton dryrun request. To simplify construction, the PR proposes moving construction intoBlackboxPyTealer.If the idea is worth keeping, then other
DryRunInspectormethod ought to be provided.Aside: I originally attempted to implement the PR by exposing a partially applied function in order to minimize parameter duplication.
DryRunInspector. I can see downsides to adding another layer, which is why I present the PR optionally.