-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactoring hydra_provider #113
Conversation
- commands service - database service - serial data feed service - random data feed service
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.
Overall looking good! Left a few comments. Some are about things outside the scope of this PR. If you don't agree with those, just ignore them. Might have a second look later this week.
VALUES ($1, $2, $3)", | ||
rocket_message_id, | ||
format!("{:?}", self.level), | ||
format!("{:?}", self.event) |
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.
format!("{:?}", self.event) | |
format!("{}", self.event) |
Not sure if this was tested and working with a Debug, but pretty sure Display should be used here.
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 forwarded this from the old code. I'm guessing it was tested? But we'll double check when we test this weekend :)
(rocket_message_id, level, event) | ||
VALUES ($1, $2, $3)", | ||
rocket_message_id, | ||
format!("{:?}", self.level), |
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.
Nitpick. While using Debug here is fine I suppose, it's usually not meant for a situation like this. Personally, I would manually map the log level to the desired string.
hydra_provider/src/database_service/hydra_input/radio_status.rs
Outdated
Show resolved
Hide resolved
hydra_provider/src/database_service/hydra_input/messages/sensor/gps_position1.rs
Show resolved
Hide resolved
|
Oh I also deleted the |
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.
o7
if it works...
@darrenrahnemoon I have approved this PR some time ago. |
Yeah we can merge. I was gonna test it from the client side to see the data showing up properly but we can do it later I suppose. |
Changes: