-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: add vanities #310
feat: add vanities #310
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
c0acc1f
to
ea13944
Compare
89528f7
to
7f37087
Compare
ca8336e
to
89f963c
Compare
89f963c
to
3f94ebd
Compare
Sorry for the churn on this. While writing out the cookbook recipes, I discovered a few improvements. This should be good to go now. |
Ok, I'm pretty sure I'm actually done now :) |
after_destroy : AfterDestroyCallback, optional | ||
Called after the Vanity is successfully destroyed, by default None |
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 interesting to me; what are its use cases?
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.
It's currently used on L145 below (self._vanity._after_destroy = self.reset_vanity
), which says, "after the vanity is destroyed, call reset_vanity". It's a way to control the state of VanityMixin without having to interact with it directly from Vanity.
class HasGuid(TypedDict): | ||
"""Has a guid.""" | ||
|
||
guid: Required[str] |
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.
Could you explain how this works? Is the internal class definition here is mostly for type-related stuff? (I see it used in the __init__
method below)
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.
Yep, it is strictly for the type checker. It makes this class easier to work with in unit tests.
|
||
session = requests.Session() | ||
url = Url(base_url) | ||
after_destroy = Mock() |
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.
Aha — is after_destroy
a parameter so that it can be mocked?
Adds job support. The mixin pattern is continued, and a 'jobs' property is added to `ContentItem.` A new abstraction around resources called `Active` is introduced. This abstraction begins transitioning to a more idiomatic approach to interacting with collections. Similar to how printing a `Resource` shows a `dict`, the `ActiveSequence` shows a familiar `List[dict]` representation. The `ActiveFinderMethods` supports the `find` and `find_by` methods. This is inspired by this Ruby on Rails module https://api.rubyonrails.org/classes/ActiveRecord/FinderMethods.html. Ideally, this separation of concerns provides a straightforward way to compose bindings in the future and reduces the amount of duplication across implementations. Additional background may be found here https://github.com/tdstein/active Related to #310 Resolves #306
Adds vanities support.
A new mixin pattern is introduced, which adds the
vanity
attributes to theContentItem
class. This abstraction is still a bit leaky since the endpoint is hardcoded to 'v1/content' . But I find this design helpful since it collocates everything regarding vanities into a single file. In addition, the Content class has started to get a little unwieldy due to its size.The use of TypedDict, Unpack, Required, and NotRequired from
typing_extensions
is also introduced. Thanks to https://blog.changs.co.uk/typeddicts-are-better-than-you-think.html, I discovered this pattern provides a much more succinct way to define pass-through arguments. It also greatly simplifies the verbosity of overload methods. That isn't shown here, but I have a test on another branch.Resolves #175