-
Notifications
You must be signed in to change notification settings - Fork 54
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
Issue #1724 Derive actor configs from TEdgeConfig #1745
Issue #1724 Derive actor configs from TEdgeConfig #1745
Conversation
@@ -77,3 +89,12 @@ async fn main() -> anyhow::Result<()> { | |||
runtime.run_to_completion().await?; | |||
Ok(()) | |||
} | |||
|
|||
fn mqtt_config(tedge_config: &TEdgeConfig) -> Result<MqttConfig, TEdgeConfigError> { |
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.
Instead of this function, I'd have preferred something like TryFrom<&TEdgeConfig> for MqttConifg
but the compiler was not letting me do that with the error: error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
.
The other option would have been either:
- Introduce a concrete wrapper impl for
MqttConifg
and implement theTryFrom
for that type - Implement the
TryFrom
for themqtt_channel::Config
itself. But, for that I need to make themqtt_channel
crate depend on thetedge_config
crate as well, which didn't seem like a good idea.
Since both were additional work for little benefit, I chose this path. But happy to fall back to either approach if you prefer one of those as well.
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'm okay with that.
A 3rd alternative is to TryFrom<&TEdgeConfig> for MqttConfig
in the tedge_config
crate. This would add many dependencies to tedge_config
but I see those less problematic.
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 will approve. Just run cargo +nightly fmt
#[derive(Clone, Debug, Default)] | ||
pub struct HttpConfig {} | ||
|
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 agree to remove this for now, even if this might be useful later. Indeed, the plan was here to set options such as pool_idle_timeout
@@ -77,3 +89,12 @@ async fn main() -> anyhow::Result<()> { | |||
runtime.run_to_completion().await?; | |||
Ok(()) | |||
} | |||
|
|||
fn mqtt_config(tedge_config: &TEdgeConfig) -> Result<MqttConfig, TEdgeConfigError> { |
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'm okay with that.
A 3rd alternative is to TryFrom<&TEdgeConfig> for MqttConfig
in the tedge_config
crate. This would add many dependencies to tedge_config
but I see those less problematic.
9cc8705
to
88c014a
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.
Approved
Robot Results
Passed Tests
|
Proposed changes
Derive actor configs from TEdgeConfig
Types of changes
Paste Link to the issue
#1724
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments