-
Notifications
You must be signed in to change notification settings - Fork 61
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
Straw man for approvers resource #14
Conversation
Addresses issue #12 |
content-api/server/main.py
Outdated
|
||
|
||
@app.route('/approvers/', defaults={'approver': None}, methods=['GET', 'POST']) | ||
@app.route('/approvers/<string:approver>', methods=['GET', 'PUT', 'DELETE', 'PATCH']) |
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.
May I suggest breaking this up into different functions for testability / readability? eg
@app.route('/approvers/<string:approver>', methods=['GET'])
def get_approvers(approver):
if approver is None:
return approvers.list()
return approvers.get(approver)
@app.route('/approvers/<string:approver>', methods=['POST'])
def post_approvers(approver):
. . .
return approvers.insert(body)
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 think it's testable as is, but it's sure a lot less readable than it could be. Will be doing this.
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.
More readable now.
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.
Requesting changes to ensure as a group we have further discussion about Cloud Run vs. Cloud Functions and Firestore vs. Cloud SQL before merging.
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.
Thanks for responding to my feedback @engelke. My takeaway is that the term strawman here is presenting this change as a "pretotype v2" for the API. In that light, the code and decisions made here are not intended as a first iteration: the lessons we're learning here are about the API design and learning how the website will integrate with this backend.
* Straw man for approvers resource * Prepare for generic resource handling * More readable, ready for another resource * Reuse cached firestore client * Instructions to run locally * Handle invalid resource names * Add donors resource * Let black format this code * Enforce optional etag matching
No description provided.