-
Notifications
You must be signed in to change notification settings - Fork 11
Add DeviceId from str conversion #348
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
Conversation
e796108 to
c75ca79
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #348 +/- ##
==========================================
+ Coverage 46.14% 46.44% +0.30%
==========================================
Files 28 28
Lines 2659 2676 +17
Branches 2659 2676 +17
==========================================
+ Hits 1227 1243 +16
Misses 1375 1375
- Partials 57 58 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
bluez-async/src/device.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl TryFrom<&str> for DeviceId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about implementing FromStr instead? That's more common for parsing things from a string.
| fn from_string() { | ||
| let device_id = DeviceId::try_from("hci0/dev_11_22_33_44_55_66"); | ||
| assert_eq!(device_id.unwrap().to_string(), "hci0/dev_11_22_33_44_55_66"); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test for converting an invalid path String to a DeviceId too, to make sure it returns an error as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i did some digging and turn out that dbus::Path returns Result<Path, Infallible> so it always succeeds and should never produce an error, so i ended up with adding simple validation (check if input starts with hci)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me that Path::try_from is an auto implementation based on Path::from, which in turn will panic if there is an invalid path. But Path::new will return an error.
bluez-async/src/device.rs
Outdated
|
|
||
| fn try_from(s: &str) -> Result<Self, Self::Error> { | ||
| let path = Path::try_from(format!("/org/bluez/{}", s)) | ||
| .map_err(|_| dbus::Error::new_failed("Invalid D-Bus path"))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to map the error here?
c75ca79 to
e4200f5
Compare
adding missing conversion from
&strresolve: #296
use case (see btleplug feature)
DeviceIdas string in databaseBaluetoothSession.get_device_info(restored_id)alternatively restore btleplug PeripheralId (wrapper for DeviceId) which i believe is referred in related issue