Merge ~ycheng-twn/plainbox-provider-resource:0001_use_hex_in_vid_pid into plainbox-provider-resource:master

Proposed by Yuan-Chen Cheng
Status: Rejected
Rejected by: Sylvain Pineau
Proposed branch: ~ycheng-twn/plainbox-provider-resource:0001_use_hex_in_vid_pid
Merge into: plainbox-provider-resource:master
Diff against target: 38 lines (+12/-3)
1 file modified
bin/udev_resource.py (+12/-3)
Reviewer Review Type Date Requested Status
Jonathan Cave (community) Disapprove
Review via email: mp+388983@code.launchpad.net

Commit message

per test, the value type int have the following attribute:

product_id:
subproduct_id:
subvendor_id:
vendor_id:

For above id, hex is the preferred format.

To post a comment you must log in.
Revision history for this message
Jonathan Cave (jocave) wrote :

It's not obvious to me whether this might break filters applied in providers - I would hope that any comparisons are made based on the numerical value rather than a string, but I wouldn't guarantee it.

This needs to be checked before it can land.

review: Needs Information
Revision history for this message
Yuan-Chen Cheng (ycheng-twn) wrote :

Sorry I don't know what exactly the test case I should go through.

Will it be accepted if hex is output only if some command line option is applied?

like --hex maybe?

Revision history for this message
Jonathan Cave (jocave) wrote :

It would be necessary to check all job definitions in all providers that use this resource.

I quick grep through all the git repositories I have cloned reveals lines like:

checkbox-provider-mcp2210/units/jobs.pxu:
template-filter: device.category == 'HIDRAW' and device.vendor_id == '1240' and device.product_id == '222'

This is a string comparison that would be broken by this change and would have to be modified. Hence, at this point I'm -1

review: Disapprove

Unmerged commits

b110cd1... by Yuan-Chen Cheng

output hex for vid and pid

Signed-off-by: Yuan-Chen Cheng <email address hidden>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/bin/udev_resource.py b/bin/udev_resource.py
2index b18e085..688a924 100755
3--- a/bin/udev_resource.py
4+++ b/bin/udev_resource.py
5@@ -37,13 +37,22 @@ attributes = ("path", "name", "bus", "category", "driver", "product_id",
6 "symlink_uuid")
7
8
9-class UdevResultDump:
10+def format_attr_val(attr, val):
11+ fmt = "{}: {}"
12+ if isinstance(val, int):
13+ if (val < 0x10000):
14+ fmt = "{}: 0x{:04x}"
15+ else: # defensive code
16+ fmt = "{}: 0x{:08x}"
17+ return fmt.format(attr, val)
18+
19
20+class UdevResultDump:
21 def addDevice(self, device):
22 for attribute in attributes:
23 value = getattr(device, attribute)
24 if value is not None:
25- print("%s: %s" % (attribute, value))
26+ print(format_attr_val(attribute, value))
27 print()
28
29
30@@ -77,7 +86,7 @@ class UdevResultLister:
31 for attribute in attributes:
32 value = getattr(device, attribute)
33 if value is not None:
34- print("%s: %s" % (attribute, value))
35+ print(format_attr_val(attribute, value))
36 print()
37
38 def addDevice(self, device):

Subscribers

People subscribed via source and target branches