-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add jobs #314
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 jobs #314
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
toph-allen
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 generally looks good — but approving with the caveat that I don't feel like I fully grok some of the mechanisms at play here. Would be great to walk through the PR together so I can ask questions.
|
@toph-allen and I met over Zoom to discuss his questions and talk about the code in more depth. Please let me know if anyone else would like to do the same. Happy to do so. |
Adds job support.
The mixin pattern is continued, and a 'jobs' property is added to
ContentItem.A new abstraction around resources called
Activeis introduced. This abstraction begins a transition to a more idiomatic approach to interacting with collections. Similar to how printing aResourceshows adict, theActiveSequenceshows a familiarList[dict]representation. TheActiveFinderMethodssupports thefindandfind_bymethods. 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