Fix MqttOptions setters

[manifest]

The changes from the PR + I've updated the usage of MqttOptions in the tests. I've also added Travis CI support here.

[manifest]

I've kept Rustfmt and Clippy jobs for Travis in case you will want to start using them in the future. At the moment some code formatting is required before we may enable them.

[manifest]

@tekjar it seems as Travis CI integration is disabled for the project.

[tekjar]

Hi. Thanks for the PR. Since this is a breaking change, I won't be able to merge it now. I'll read up a bit on builder patterns in rust and then take a decision. Is there a problem with the below approach 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)

[manifest]

@tekjar I think we should decide first which approach we want to use for MqttOptions in rumqtt, because the implementation we have now isn't a Builder pattern.

There are two different approaches how you can modify properties of the struct in Rust

1. Using Builder pattern
2. Using setters

Here is a good overview of possible implementations of Builder pattern in Rust https://doc.rust-lang.org/1.0.0/style/ownership/builders.html

In common case we consume current instance of Self and return a new instance from the modifier function

fn property(self, value: T) -> Self {
    Self {
        property: value,
        ..self
    }
}

We use it like that

MqttProperties::new()
    .clean_session(value)
    .keep_alive(10)
    .build();

Setters and getters approach looks like that

fn set_property(&mut self, value: T) -> &mut Self {
    self.property = value;
    self
}

fn property(&self) -> T {
    self.property
}

We use it like that

let mut opts = MqttProperties::new();
opts.set_clean_session(value);
opts.set_keep_alive(10);

Basically if we need getters we should go with pairs of getters and setters. If we need a way just to construct the MqttOptions or Client we need to use Builder pattern.

[manifest]

I can change implementation to that implements Builder pattern if you prefer it. But we will have to remove getters to follow best practicies.

[tekjar]

the implementation we have now isn't a Builder pattern

Hi. Didn't mean to say that what we have now is a builder pattern. I want to understand the common way of doing it.

Maybe use crates like this to avoid boilerplate?

Maybe the same thing applies for connection method and security options because of more features like this

[manifest]

I suggest to fix one problem at a time. Currently, there is a bug in MqttOptions.

I mean it shouldn't be the code like this:

let opts = if let Some(value) = config.clean_session {
    opts.set_clean_session(value)
} else {
    opts
}

This PR is aimed on this problem. If we decide to use Builder Pattern, probably it would make sense to introduce ClientBuilder instead of MqttOptions. It could be a next step.

One problem at a time. The more releases the better.

[manifest]

Maybe use crates like this to avoid boilerplate?

We have just a few attributes here. I would personally prefer explicit implementation making the code more readable. Implementing a builder you also may want to process input arguments differently, convert values, etc.

Maybe the same thing applies for connection method and security options because of more features like this.

It's unusual to see add_ prefixes for builder's methods.

[tekjar]

Currently, there is a bug in MqttOptions

I don't understand why you consider this a bug.

I mean it shouldn't be the code like this:

And why shouldn't it? It might be that this isn't a standard way of building options but definitely not a bug IMO

I can change implementation to that implements Builder pattern if you prefer it

I suggested derive_builder since you offered to implement builder patterns.

This PR is aimed on this problem One problem at a time. The more releases the better

This is already a breaking change. I would rather design this for the next major release instead of accepting this PR now only to change it again

[manifest]

I don't understand why you consider this a bug.

I consider this as a bug because we can't write the code:

if let Some(value) = config.clean_session {
    opts.set_clean_session(value)
}

Instead, we required to write code like that:

opts = match config.clean_session {
    Some(value) => opts.set_clean_session(value),
    _ => opts,
};

That doesn't make any sense.

[manifest]

This is already a breaking change. I would rather design this for the next major release instead of accepting this PR now only to change it again

Can I help somehow?

[tekjar]

I'll modify these to builder patterns before next release. But I, unfortunately, won't be able to spend any time this month. I'll give this a shot next month and then ping you :)

[manifest]

unfortunately, won't be able to spend any time this month

I could do that if you clarify the way you see it.

Something like that?

impl MqttOptionsBuilder {
    ...
    fn build() -> MqttOptions {
    }
}

[tekjar]

Yeah. Something like that + Getter methods. Builder patterns preferably with crates like derive_builder

[manifest]

@tekjar please, take a look. I've added MqttOptionsBuilder and replaced panics with builder's errors.