Add external USB storage devices

[lezmaka]

This gets information about external USB storage devices connected to the NAS. My hope is that this will allow Home Asistant to show USB storage device status.

• fixes Add external USB drive information #238

[mib1185]

Hi @lezmaka

many thanks for bringing this is 🤝
I've left some comments, but at all I think we should also have a class for the external USB device itself and put all properties to this.

[lezmaka]

Hi @lezmaka

many thanks for bringing this is 🤝 I've left some comments, but at all I think we should also have a class for the external USB device itself and put all properties to this.

I'm not sure if I understand, so just want to ask. Are you wanting it to be similar to download station? For example, the SynoCoreExternalUSB I created would only have update, get_devices, device_ids, and get_device. (and move this code into init.py?). Then a separate class named maybe SynoCoreExternalUSBDevice that has device_name, device_type, device_size_total, etc.?

And sorry to ask but since I'm new, I make these changes in the branch I made, yes? Then when I push changes to GitHub, does the PR get updated automatically?

Thanks for being patient with me :)

[mib1185]

I'm not sure if I understand, so just want to ask. Are you wanting it to be similar to download station? For example, the SynoCoreExternalUSB I created would only have update, get_devices, device_ids, and get_device. (and move this code into init.py?). Then a separate class named maybe SynoCoreExternalUSBDevice that has device_name, device_type, device_size_total, etc.?

SynoCoreExternalUSBDevice sounds suitable 👍
and yes, the SynoCoreExternalUSB should only care about updating the data from api and returning the devices as objects. Further the device objects will contain its properties and a list of partition objects, which implements there own properties. Further this all should be held within the external_usb.py module file

And sorry to ask but since I'm new, I make these changes in the branch I made, yes? Then when I push changes to GitHub, does the PR get updated automatically?

exactly 👍

[lezmaka]

Thanks!

I had a couple more questions. When making changes requested for a PR, should I avoid making other changes that don't relate to the comments from the reviewer or haven't been discussed?

• Spelling errors
• Renaming properties e.g. producer to device_producer
• Update test data and tests. Related to your comments above, adding tests for handling a drive with no partitions or parition is not formatted

Also, should Python property names match the data the API returns? The API property name for the manufacturer of the device is called "producer" but I might want to call the Python property "manufacturer" or "device_manufacurer".

[mib1185]

Adding changes/commits related to the review comments is absolutely OK.
Adding completely new features (not related to the review comments) should avoided or at least pointed out as such.

There is not rule, that the property name should be equal to the API data, but the API is not used to be visible to the user, therefore using more common terms is ok and i would prefer manufacturer over producer, too 😉

[lezmaka]

I think what's causing me to second-guess everything is Python seems foreign to me - and so simple things like what should be a property and what should be a function aren't clear. Most of my experience is with C# in Windows and ASP.NET. For the function/property question, should I try to imagine what I might do in C#?

[mib1185]

Hi @lezmaka
I did not forgot you, but was busy with another new feature (Syno Photos) which is going to be integrated in this lib and HA.
I assume to review your PR within the next week 🤞

[lezmaka]

Thanks for the update. I understand that not everything can be done immediately or multiple things at once. Plus I'm guessing since I'm new to all of this that my PR might need more work than others :)

[mib1185]

Hi @lezmaka
I did not do a functional test right now, but already had a look into the code itself and left some comments.

[lezmaka]

Thanks for taking a look! I'll go through your comments and make some changes.

I definitely understand waiting to test until you're pretty sure everything looks good. I'm at the point with Python where I can get things to "work" and I can use it, but that doesn't mean it isn't fragile or works in a way that makes sense to others. I still have the USB drives connected to my NAS so if there's something you would like me to test, let me know and I'll see what I can do.

[lezmaka]

Hi @mib1185
When should I use the "Resolve conversation" button on the comments? Assuming I don't have any questions, should I mark something resolved once I've made my change locally, or should I wait until I've pushed the change to GitHub? Or is resolving something the reviewer should typically handle?

[mib1185]

When should I use the "Resolve conversation" button on the comments?

at least the corresponding changes should be pushed to Github. It is also possible, that a comment ends up in a discussion, which could resolve the comment.

I still have the USB drives connected to my NAS so if there's something you would like me to test, let me know and I'll see what I can do.

If you own an usb printer, it would also be great to test, if this does not interfere this new feature. Further it would be great to have some negative tests, like nothing is connected to usb or it is connected, but already released in DSM or a usb stick without any partition

Thanks for your efforts and no worries about something could be done in a wrong way, there are always multiple ways do achieve something;)

[lezmaka]

I pushed some changes, though I wasn't able to figure everything out.

If you own an usb printer, it would also be great to test, if this does not interfere this new feature. Further it would be great to have some negative tests, like nothing is connected to usb or it is connected, but already released in DSM or a usb stick without any partition

I do have a usb printer. I connected it, made sure DSM saw it, and it didn't have any effect on the data returned by the API. When a device is plugged in to the NAS but released in DSM, it isn't returned at all. If nothing is connected, the API returns an empty array. I added the response when nothing is connected to const_7_core_external_usb.py but I couldn't figure out how to get a second test for the same API key.

I'm sure there are some things I missed, couldn't figure out, or made a wrong change. Let me know what else needs to be changed.

[mib1185]

Hi @lezmaka

many thanks for the detailed test with a usb printer 👍 and especial for the efforts spent to this PR and bring this feature in 🤝
Because, meanwhile I finished the introduction of type annotations (#183) in the whole library, I've add the type annotations also in here (fa87796)

Overall from my point of view, this PR is ready to be merged. If you have anything else to add here, than now 😁

[lezmaka]

I forgot to change the example usage in the readme, just pushed an update with changes for that. Other than that, I agree that it's ready. And thank you for handling the type annotations, not sure how long it would have taken me to figure that out. 🙂

I appreciate your patience guiding me through the whole process!