-
Notifications
You must be signed in to change notification settings - Fork 9
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: interest registration endpoint #246
feat: interest registration endpoint #246
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.
LGTM once you add some tests. Should be a simple copy past edit of the existing tests.
Thanks! And yes, haha, was planning to add unit tests before marking the PR ready 😁 (after I get the e2e tests passing) |
7a3b8d8
to
8123763
Compare
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.
api/src/server.rs
Outdated
stream_id: Option<String>, | ||
_context: &C, | ||
) -> Result<InterestsSortKeySortValuePostResponse, ApiError> { | ||
debug!( |
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.
nit, this debug is redundant as the params are already part of the tracing context and will get logged at debug when the function returns successfully or logged at error in the case of an error.
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.
Ah nice, ok, taking it out.
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.
LGTM, one comment but I think it can be handled separately.
stop_builder.with_controller(&controller).with_max_init(), | ||
), | ||
(None, Some(_)) => { | ||
return Err(ApiError( |
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 not a pattern we've been using, but we could add a 400 response and return e.g InterestsSortKeySortValuePostResponse::BadRequest
instead of a 500 for things like this. Might be nice to improve the API responses across the board, potentially as a separate ticket. I know changing this would require some refactoring given how this code is shared.
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.
Yeah, I like that, it's more aligned with how the status codes should be used.
No description provided.