-
Notifications
You must be signed in to change notification settings - Fork 23
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
Save routing fees in database #949
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 👍
I left minor remarks, but nothing critical to action.
|
||
#[derive(Insertable, Debug, PartialEq)] | ||
#[diesel(table_name = routing_fees)] | ||
struct NewRoutingFee { |
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.
🤔 is it really necessary to have a separate insertable model for this simple data?
Personally I would prefer one model as I guess it will also reduce complexity.
}; | ||
|
||
if let Err(e) = db::routing_fees::insert( | ||
NewRoutingFee { |
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.
Since the struct NewRoutingFee
is only used here, I think you can remove it and pass in the fields directly into this function.
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.
My timebox for addressing this expired :)
Diesel kept annoying me when I tried to safe the values directly using a tuple. I don't think it's worth investing more time into fighting with Diesel.
I generally think it's OK to use the same pattern for the tables and accept this bit of boilerplate. If you think it's important I'm happy to address it in a follow up.
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
404dfa1
to
044e726
Compare
cf623c3
to
a87dcb5
Compare
a87dcb5
to
8844db0
Compare
8844db0
to
2246001
Compare
bors r+ |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page.
|
fixes https://github.com/get10101/meta/issues/177
Notes: