Skip to content
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

refactor Mqttoptions #789

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

refactor Mqttoptions #789

wants to merge 8 commits into from

Conversation

swanandx
Copy link
Member

This PR aims to improve usability of Mqttoptions and fixes #434 and #270 . some of the changes are:

  • port isn't required argument to new():

    • It can be passed in broker address, e.g. localhost:1337, if not specified, defaults ports will be used as per MQTT specifications.
    • set_port() can be used to set/override the port
  • Passing partial url as address works:

    • localhost, localhost:1883, ws://localhost, mqtts://localhost:1884, wss://example.org/mqtt etc. are all valid addresses
    • passing invalid addresses like swanx://localhost, localhost:999999, localhost:x2 return errors regarding scheme / port.
  • Same behavior of MqttOptions for both tcp and webscokets :)

Issues / queries:

  • instead of returning Error from MqttOptions::new(), shall we panic if provided options are wrong?
    why?: because we do same for other fns like set_keep_alive / set_inflight.
    why not?: rumqttc is used as library, returning error so users can handle em as they want sound better than panics.

  • we aren't performing any host validations, i.e. MqttOptions::new("client_id", "[@-1") won't return any error. Rather it would try to connect to the host and fail with lookup error:
    Io(Custom { kind: Uncategorized, error: "failed to lookup address information: Name or service not known" }). Shall we do strict hostname checking?
    why?: we would catch some of the invalid address early ( like empty hostname, invalid IPv6 etc.)
    why not?: It is hard to validate! using other dependency like url::Host seems good but it doesn't support no_std which we would like to have :(

Please comment your thoughts, I will mark the PR as ready once above/any other queries are resolved :)

Type of change

  • Breaking change

Checklist:

  • Formatted with cargo fmt
  • Make an entry to CHANGELOG.md if it's relevant to the users of the library. If it's not relevant mention why.
    TODO: once we have finalized it!

@swanandx swanandx mentioned this pull request Feb 2, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MqttOptions::new should be refactored
1 participant