-
Notifications
You must be signed in to change notification settings - Fork 29
Replace API Server's mock data responses with database calls #1190
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
Conversation
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.
Actually this is looking good as is. I'm happy to merge it.
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.
Looks good. A tiny style comment below:
state | ||
.db | ||
.transaction_ro() | ||
.map_err(|_| { | ||
APIServerWebServerError::ServerError( | ||
APIServerWebServerServerError::InternalServerError, | ||
) | ||
})? |
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 define a constant or something for the error? I don't like how a mere conversion of error type takes up 5 lines while the actual "substantive" code is a fraction of that.
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 replicated many times over.
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. There's currently not that many error types, but once it's fully fleshed out, I'll replace the errors with real constants.
Most of the current API server endpoints are now pulling from a database handle.