Added documentation about service name length <=15 accoring to #427 #431
Conversation
Added two comments to explain the limitation and that the lib will silently fail without a monitor connection - Added a comment to the ty_domain property of ServiceInfo - Added a bold portion in the docstring of new
If this fails no service will be published and the library is not working as intended by the user therefore this should be a hard error. There are a ton of logs and with debug it seemed unimportant and was near impossible to find for users without experience of this crate
keepsimple1
left a comment
keepsimple1
left a comment
There was a problem hiding this comment.
Overall looks good! Thanks for your PR! A couple of comments inline.
| //! // Browse for a service type. | ||
| //! // ❗ Make sure that the service name: "mdns-sd-my-test" is <= 15 characters or this will silently fail | ||
| //! // You may also want to setup a monitor connection via `mdns.monitor` and log any `DaemonEvent::Error` | ||
| //! // You can see how to setup this inside the register example |
There was a problem hiding this comment.
I don't think we need to add this doc comment for browse as in this case the service name is provided for the caller, not decided by the caller.
There was a problem hiding this comment.
Yes agreed. I thought that as well but I was unsure what happens if someone overwrites the max length and publishes a service with more than 15 chars. Will that be found by browse or will it fail?
There was a problem hiding this comment.
yes it will be found by browse.
| //! let mdns = ServiceDaemon::new().expect("Failed to create daemon"); | ||
| //! | ||
| //! // Create a service info. | ||
| //! // ❗ Make sure that the service name: "mdns-sd-my-test" is <= 15 characters or this will silently fail |
There was a problem hiding this comment.
nitpick: as we do allow the change of the limit via set_service_name_len_max, we should probably say "no longer than the max length limit (15 characters by default)".
| // Check the service name length. | ||
| if let Err(e) = check_service_name_length(info.get_type(), self.service_name_len_max) { | ||
| debug!("check_service_name_length: {}", &e); | ||
| error!("check_service_name_length: {}", &e); |
There was a problem hiding this comment.
👍 Thinking about this more, I tend to agree that it's valid to log it as error because this is truly a case of failure.
| /// "_my-service._udp.local.". | ||
| /// With default settings service name length must be <= 15 bytes | ||
| /// so "_abcdefghijklmno._udp.local." would be valid but "_abcdefghijklmnop._udp.local." is not | ||
| /// ❗ **This will silently fail and no error will be shown** unless you setup a monitor connection via `ServiceDaemon::monitor()` |
There was a problem hiding this comment.
Now we have an error level log, this can be updated.
2 participants
Issue: #427
Changes made:
Changes not made but possible if desired: