Add FastlyResource Base Class#47
Merged
Merged
Conversation
erikrose
approved these changes
Feb 21, 2026
Member
erikrose
left a comment
erikrose
left a comment
There was a problem hiding this comment.
I think that's a nice win. LGTM!
| @@ -0,0 +1,104 @@ | |||
| """Internal base class for Fastly resource wrappers | |||
Member
There was a problem hiding this comment.
If we call this _resource.py, then we don't have to scream at the read so much about it being internal. :-)
| penalty.add("192.168.1.1", ttl=600) # Block for 10 minutes | ||
| """ | ||
| self._box.add(entry, ttl) | ||
| self._inner.add(entry, ttl) |
Member
There was a problem hiding this comment.
I'd love to get a noun in there, since we're often referencing it at some level of remove. _wit_resource, _inner_resource, something like that.
Member
Author
There was a problem hiding this comment.
Updating to _wit_resource
erikrose
reviewed
Feb 23, 2026
| def __init__(self, store: wit_config_store.Store): | ||
| """Private constructor. Use ConfigStore.open() instead.""" | ||
| self._store = store | ||
| super().__init__(store) |
Member
There was a problem hiding this comment.
We should be able to omit this method.
erikrose
reviewed
Feb 23, 2026
| def __init__(self, counter: wit_erl.RateCounter): | ||
| """Private constructor. Use RateCounter.open() instead.""" | ||
| self._counter = counter | ||
| super().__init__(counter) |
Much of the API functinoality is wrapping and providing documentation and some basic translation on top of an underlying (patched) wit resource. With that wrapping, however, we need to consistently add context manager and close() support. This change moves that common functionality to an abstract base class with generic types so we continue to have strong typing in our implementations interactions with the "_inner" WIT resource.
- Remove unecessary constructor wrapper calls - Rename _inner to _wit_resource - Rename module to _resource from resource as private, remove some of the internal disclaimers.
posborne
force-pushed
the
posborne/fastly-resource-abc
branch
from
e231e99 to
572fc68
Compare
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
2 participants
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.
Add FastlyResource base class for common concerns
Much of the API functinoality is wrapping and providing documentation
and some basic translation on top of an underlying (patched) wit
esource. With that wrapping, however, we need to consistently add
context manager and close() support.
This change moves that common functionality to an abstract base class
with generic types so we continue to have strong typing in our
implementations interactions with the "_inner" WIT resource.
Targeting at the ERL base for now to make review easier; if/when that merges I'll change the base to main. I still have mildly mixed feelings about using inheritance here, but it seems worthwhile to avoid having to repro the docs everywhere.