Skip to content
This repository has been archived by the owner on Jan 17, 2020. It is now read-only.

Fix MqttOptions setters #144

Merged
merged 2 commits into from Mar 22, 2019
Merged

Conversation

manifest
Copy link
Contributor

@manifest manifest commented Mar 21, 2019

The following code fails because the current implementation of setters require implementation of Copy trait.

let mut opts = rumqtt::MqttOptions::new(client_id, host, port.as_u16());
if let Some(value) = config.clean_session {
    opts.set_clean_session(value);
};

Ok(opts)
error[E0382]: use of moved value: `opts`
   --> /Users/manifest/projects/netology-group/svc-agent-rs/src/mqtt.rs:107:12
    |
103 |             opts.set_clean_session(value);
    |             ---- value moved here
...
107 |         Ok(opts)
    |            ^^^^ value used here after move
    |
    = note: move occurs because `opts` has type `rumqtt::mqttoptions::MqttOptions`, which does not implement the `Copy` trait

error: aborting due to previous error

For more information about this error, try `rustc --explain E0382`.
error: Could not compile `svc-agent`.

To learn more, run the command again with --verbose.

@manifest
Copy link
Contributor Author

manifest commented Mar 21, 2019

I've also changed the type of keep_alive option to u64 since it is expected by Duration constructor and to make it symmetrical to other options using time intervals in seconds as reconnect.

@tekjar tekjar merged commit f295839 into AtherEnergy:master Mar 22, 2019
@tekjar
Copy link

tekjar commented Mar 22, 2019

Thank you :)

@manifest
Copy link
Contributor Author

manifest commented Mar 22, 2019

@tekjar Could we bump a new crate version? We need this change in another crate, so we can't use branch or just depend on the commit there.

@tekjar
Copy link

tekjar commented Mar 22, 2019

Ahh, I'm not sure if there are any breaking changes in the master branch since the last release. Can you let me know if there are any? I'll do minor increment if not

@manifest
Copy link
Contributor Author

manifest commented Mar 22, 2019

Can you let me know if there are any?

There is none of breaking changes as far as I can see. We can go with update of the minor version.

@tekjar
Copy link

tekjar commented Mar 22, 2019

Looks like this is breaking a bunch of tests and examples. I might have missed the CI here. We have to revert this

@tekjar
Copy link

tekjar commented Mar 22, 2019

I'll revert this now. I need some consensus on builder patterns. Can you use this for now?

let mut opts = rumqtt::MqttOptions::new(client_id, host, port.as_u16());
let opts = if let Some(value) = config.clean_session {
    opts.set_clean_session(value)
} else {
    opts
}

Ok(opts)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants