Pci class - #9
Conversation
There was a problem hiding this comment.
Code Review
This pull request extends the PCI class in hwdata.py to support parsing and retrieving PCI device classes, subclasses, and programming interfaces. It also fixes several typos in docstrings and updates example.py to demonstrate the new functionality. Feedback focuses on improving the efficiency and idiomatic nature of dictionary membership tests by removing redundant list(keys()) calls and using the 'not in' operator as per PEP 8.
| else: | ||
| device_class = l[1].lower() | ||
| device_class_name = ' '.join(l[2:]) | ||
| if not device_class in list(PCI.devices_class.keys()): |
There was a problem hiding this comment.
Membership testing in a dictionary is more efficient when done directly on the dictionary object. Also, per PEP 8, item not in dict is preferred over not item in ....
| if not device_class in list(PCI.devices_class.keys()): | |
| if device_class not in PCI.devices_class: |
References
- PEP 8 recommends using the 'not in' operator for membership tests and avoiding redundant list creation for dictionary key checks. (link)
| """ | ||
| device_class = device_class.lower() | ||
| if self.cache: | ||
| if device_class in list(PCI.devices_class.keys()): |
There was a problem hiding this comment.
Redundant use of list(...keys()). Membership testing should be performed directly on the dictionary for better performance and adherence to Python idioms (PEP 8).
| if device_class in list(PCI.devices_class.keys()): | |
| if device_class in PCI.devices_class: |
References
- PEP 8 recommends avoiding redundant list creation for dictionary key checks. (link)
| if device_class in list(PCI.devices_class.keys()): | ||
| if subclass in list(PCI.devices_class[device_class][1].keys()): |
There was a problem hiding this comment.
Redundant use of list(...keys()) for membership testing. Checking directly against the dictionary is more efficient and follows PEP 8 guidelines.
| if device_class in list(PCI.devices_class.keys()): | |
| if subclass in list(PCI.devices_class[device_class][1].keys()): | |
| if device_class in PCI.devices_class: | |
| if subclass in PCI.devices_class[device_class][1]: |
References
- PEP 8 recommends avoiding redundant list creation for dictionary key checks. (link)
| if device_class in list(PCI.devices_class.keys()): | ||
| if subclass in list(PCI.devices_class[device_class][1].keys()): | ||
| if prog_if in list(PCI.devices_class[device_class][1][subclass][1].keys()): |
There was a problem hiding this comment.
Multiple redundant calls to list(...keys()). These membership checks are more efficient when performed directly on the dictionary objects, as per PEP 8.
| if device_class in list(PCI.devices_class.keys()): | |
| if subclass in list(PCI.devices_class[device_class][1].keys()): | |
| if prog_if in list(PCI.devices_class[device_class][1][subclass][1].keys()): | |
| if device_class in PCI.devices_class: | |
| if subclass in PCI.devices_class[device_class][1]: | |
| if prog_if in PCI.devices_class[device_class][1][subclass][1]: |
References
- PEP 8 recommends avoiding redundant list creation for dictionary key checks. (link)
| while True: | ||
| line = f.readline() | ||
| if not line: | ||
| break |
There was a problem hiding this comment.
Why did you decide to change the for-loop to while-loop?
There was a problem hiding this comment.
i did this a few months ago, i'm trying to remember why. i think it was because i've duplicate the parsing code to avoid having to fully understand it, since the pci class part is the same format. i can take a closer look to make it more logical
There was a problem hiding this comment.
ok now i remember, the while-loop was for using f.readline() instead of for f.readlines(), so i can use the same loop twice without changing too much code. using readlines() force you to read the entire file in one loop. readline() retain the read offset so you can continue to read the file in another loop after a break.
do you want me to change it ?
| l = line.split() | ||
|
|
||
| if line.startswith('# C class'): | ||
| break |
There was a problem hiding this comment.
Why? Ah. I see now why. But can you document the "why" in the comment?
Hello,
i've added the possibility to get the information of "PCI Class" from pcis.ids database