bytebeamio
Improve MqttOptions creation
Improvements:
- typed-builder
- ClientId struct
- deprecated old
MqttOptions::newfor easier migration
I dont think its that bad to expose the MqttOptions to the public. The types itself prevent a lot of wrong content (maybe types like NonZeroU16 could improve that even further). I refactored the client id check into ClientId so it is checked by itself.
There are still a few open questions.
- I set the
broker_addrandporttolocalhostand1883. Especially the port sounds sane to me. Should this be kept? - It would be nice to have some generator for ClientId. Like
rumqttc-<systemtime>or so to connect easier to some broker when the client id does not need to be something fix. But this should be a separate PR. - The content of the options is now completely public. Should be rename some of them now to something better or should we keep them like that?
MqttState::newgot the panic which was previously inMqttOptions::set_inflight. Personally I think there shouldnt be any panic in a library (only impossible ones likeunreachable!()). Should we change that behaviour there towardsResult<MqttState, SomeError>or keep the panic for now?
I am open for feedback here. When only part of the improvements is liked I can create another PR with only that part.
A lot of the tests fail since the url dependency is optional, only enabled with features = ["url"]
Edit after the CI reminded me: How to handle the code example in the docs which fails without the url feature?
Yeah, I know. But I am not sure how I want to handle that well and am open for ideas there.
A lot of the tests fail since the
urldependency is optional, only enabled withfeatures = ["url"]
Maybe more details there: The parse_url method had a #[cfg(feature = "url")]. I migrated that towards an impl FromStr which also has the feature check there. The documentation that this exists was on the parse_url method which worked fine. As the documentation is kinda useless for the impl FromStr method (the user will never see it) I moved it to the main struct. But then there is no feature check there. We could mark these code examples as ignore but then they aren't tested anymore (even when the feature is enabled). These code examples could also be added somewhere else to ensure they are working but its easy for them to get out of sync. This might be a simple way to do it but its definitely not idea.
We could also write the docs twice and use conditional compilation to decide which docs to use. Then the code will only be there when the feature is enabled and only tested in that case. But I think its only possible when the docs are in a file somewhere?
I tried doing this and it worked:
rumqttc/src/lib.rs --- Rust
367 /// 367 ///
368 /// ``` 368 /// ```
369 /// # use rumqttc::MqttOptions; 369 /// # use rumqttc::MqttOptions;
... 370 /// # #[cfg(feature = "url")]
370 /// # fn main() -> Result<(), Box<dyn std::error::Error>> { 371 /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
371 /// let options = "mqtt://example.com:1883?client_id=123".parse::<MqttOption 372 /// let options = "mqtt://example.com:1883?client_id=123".parse::<MqttOption
... s>()?; ... s>()?;
372 /// # Ok(()) 373 /// # Ok(())
... 374 /// # }
... 375 /// /// # #[cfg(not(feature = "url"))]
... 376 /// # fn main() {
... 377 /// // ENABLE feature == "url"
373 /// # } 378 /// # }
374 /// ``` 379 /// ```
375 /// 380 ///
376 /// You can also go from an [`Url`](url::Url) directly: 381 /// You can also go from an [`Url`](url::Url) directly:
377 /// 382 ///
378 /// ``` 383 /// ```
379 /// # use rumqttc::MqttOptions; 384 /// # use rumqttc::MqttOptions;
380 /// use std::convert::TryFrom; 385 /// use std::convert::TryFrom;
381 /// # use url::Url; 386 /// # #[cfg(feature = "url")]
382 /// # fn main() -> Result<(), Box<dyn std::error::Error>> { 387 /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
... 388 /// # use url::Url;
383 /// let url = Url::parse("mqtt://example.com:1883?client_id=123")?; 389 /// let url = Url::parse("mqtt://example.com:1883?client_id=123")?;
384 /// let options = MqttOptions::try_from(url)?; 390 /// let options = MqttOptions::try_from(url)?;
385 /// # Ok(()) 391 /// # Ok(())
... 392 /// # }
... 393 /// # #[cfg(not(feature = "url"))]
... 394 /// # fn main() {
... 395 /// // ENABLE feature == "url"
386 /// # } 396 /// # }
387 /// ``` 397 /// ```
388 /// 398 ///
Uh, that is both smart and a simple solution! I like that!
Have you checked out the rest of the improvements? Any thoughts on them?
have you checked out the rest of the improvements? Any thoughts on them?
Yeah, they look good, especially the builder pattern, sweet. But I do think we should not be deprecating methods such as new() and parse_url() right away, this is just what I think.
As for the failing tests on windows, they are an enigma to me as well. Will need to give it a closer look, will do so in a week or two, once I am back from my break.
But I do think we should not be deprecating methods such as
new()andparse_url()right away, this is just what I think.
I like to have only one preferred way to do things. This simplifies understanding code and problems of people and simplifies debugging / performance improvements efforts.
Keeping the old methods and marking them as deprecated shows how to migrate to the new preferred way but keeps things working. Normally this shouldnt be a breaking change.
In this case it is a breaking change anyway as methods like set_keep_alive are removed completely. (See diff of the examples). So the deprecation hint is more like a helper to find the new way.
Having two ways of doing things is a mess in my eyes but if that's preferred I can provide PRs for the distinct changes.
(Tried to solve merge conflicts and accidentally merged in main instead of master → rebased my changes to get rid of the wrong merge)
I just realized there is another MqttOptions for the v5 module. Also this PR is just ignored for months now. Not sure if there is any point in keeping it up as it would also need some work in the v5 module now.
I would be happy to see improvements to the MqttOptions creation to provide easier support for multiple ways to connect to a broker especially simplifying configuration via CLI arguments but I am not sure if this helps with that. I guess with the v5 module this got even harder now?
Additional to my last comment MqttOptions seems to have even more problems which should be refactored in my opinion. (See #434)
As there need to be changes on a bigger scale than this PR, I will close this PR as it misses its point.