✨ Adapter for Windows #9
Conversation
initial work
Draft: data model definition
Rationale is that detectors will probably use some os specific imports, it's best to handle that at the top of the import stack.
Brings back `interface firmware`
windows specific dependency
Ways to get data about usb devices from Windows APIs
new parsing
more tests
some more work
Aggregating the data about disks
Added a test for system data
Added a way to know if a structure is undefined
| @property | ||
| def filter(self) -> Optional[str]: | ||
| """Filter applied on the list of elements.""" | ||
| return None |
There was a problem hiding this comment.
Tried to improve the documentation
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ComponentDescriptor(ABC): |
There was a problem hiding this comment.
I'm really struggling to understand what this object does and why we need all of this complexity. It seems like some sort of abstract factory class which instantiates a NamedTuple in a bunch of different ways, with some unexpected uses of @classmethod. Do we really need to make this so complex? I'd prefer some simple factory functions over this abstract factory (if that's what this is).
There was a problem hiding this comment.
I radically changed the code to make it simpler to follow. Hopefully, I succeeded.
There was a problem hiding this comment.
Now that I understand why hashing is necessary - I think it should be the responsibility of a single utility class to glue fetching and merging data together. In my mind this does not belong on a parent class that pollutes namespaces of all its children via inheritance. This functionality should be encapsulated and collaborate or be part of WindowsComponentDescriptor I suggested above. It's sole responsibility should be to crunch data returned by win32 lib and merge it.
|
|
||
|
|
||
| class UsbIdentifier( | ||
| named_tuple_with_defaults( # type:ignore |
There was a problem hiding this comment.
This is the only place named_tuple_with_defaults is ever used, also it sets all the values to None. Do you think it's necessary? Why would UsbIdentifier full of None even exist (be instantiated)?
There was a problem hiding this comment.
it does not set all the values to None. Just the ones which were not provided. The reason this is done is that there is no rule about what elements are present in the DeviceID and what isn't
Refactoring based on review comments (part 1)
renames and reformatting
Refactoring based on review comments (part 4)
|
Due to the amount of review comments, I refactored drastically the code and will try to answer most change requests in this comment.
|
|
@acabarbaye I'm sorry, but I'm not willing to review this again. I still see things I'm unhappy about, but I don't find it productive to work against such big diff. I think we should revisit the approach we take with regards to pull requests, especially that |
Description
New adapter for Windows.
Caveat:
Some changes may be needed for ST boards as the serial port does not seem to be retrieved properly since I updated the USB drivers with Mbed Studio. Would be worth testing on a fresh Windows machine.
Test Coverage