-
Notifications
You must be signed in to change notification settings - Fork 1
Exception fluency monkeypatching #30
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
Changes from all commits
61f45e0
3469497
2b76077
1894e1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,3 +5,9 @@ | |
|
|
||
| # Testing utilities are available but not imported by default | ||
| # Users can import them explicitly: from fastly_compute.testing import ViceroyTestBase | ||
|
|
||
| from fastly_compute.runtime_patching.patches import patch | ||
|
|
||
| # Before anything from the fastly_compute package is used, do our monkeypatching | ||
| # to make the WIT-generated code act more Pythonically: | ||
| patch() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still uneasy about doing this kind of runtime patching. It introduces magic that is not necessary without any benefit other than reducing the amount of code that needs to be generated slightly. The alternative I still believe is preferred is to have a generated layer that does the translation; this is similar to what we are patching in at runtime but generated a compile time. Use would look like this: # Current, use wit_world import that gets magically patched
from wit_world.imports import compute_runtime
# Using generated wrapper (details flexible)
from fastly_compute.wit import compute_runtimeWith that in mind, I also think we can move forward with the change as-is and it will not preclude us from changing our approach on this later on. Both would perform a similar transform, sharing most of the same code. I'll keep thinking on this, but wanted to speak my peace that this feels like a bit of a hack that could cause us some pain and it isn't necessary to achieve what is being achieved here.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noted! I'm not ordinarily a monkeypatching kind of guy either, but I think in this case the good outweighs the bad. My motivating good is that it effectively clears the undesirable behavior out of the system; no one is going to call a low-level Err-raising routine by accident. That wouldn't ordinarily be worth much worry, except that many of our routines are methods on resources (on classes, in the Python world). Any source-level wrapping would have to carry around a duplicate of each class to house nice-exception-raising methods. Now you've got 2 different A halfway approach might be to throw some kind of warning if It might not be a bad idea to convert this into a "2-way door" by importing everything in |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| """Top-level exceptions emitted by the Fastly API""" | ||
|
|
||
|
|
||
| class FastlyError(Exception): | ||
| """Abstract base class for all errors raised by Fastly APIs | ||
|
|
||
| This allows catching all errors emanating from Fastly APIs at once. | ||
| """ | ||
|
|
||
|
|
||
| class UnexpectedFastlyError(FastlyError): | ||
| """An error arising from a Fastly API but of an unanticipated kind, such | ||
| that we merely package up the low-level error and send it along. | ||
|
|
||
| Any of these encountered in the wild means we neglected to keep our Python | ||
| wrappers up to date with the WIT. | ||
| """ | ||
|
|
||
| def __init__(self, error_value: object): | ||
| """Construct. | ||
|
|
||
| :arg error_value: The ``value`` attr of the raised ``Err`` | ||
| """ | ||
| self.value = error_value | ||
|
|
||
|
|
||
| # I went with the exact verbatim names of the error cases, not appending "Error" | ||
| # to the ends of the ones that didn't have it to make them strictly conform to | ||
| # Python conventions. "except HttpInvalid" reads fine to me. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| """Monkeypatches (and supporting machinery) which make WIT behavior more Pythonic""" |
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.
I would probably be OK and it may make for easier reference to include this generated code in tree given that the inputs probably don't change too frequently and we probably do want to examine changes that occur fairly closely.
When to do regen then becomes a bit different, potentially; perhaps do it manually with some kind of CI check to catch it containing non-cosmetic differences. Not a hill I'd die on, but I do think this could be a case where including the generated code might be worthwhile to aid reference.
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.
100% agree. I was wishing for that as I developed it, so I'll open a follow-up PR with that change. It's a fairly elegant way of putting this code under test.