Closed ddemidov closed 8 years ago
Mostly looks good. The one issue I see is that I don't think it manages the address
of the devices it creates. If the device was copied from in3
, that is how it will show up in the device tree in tests.
I think it needs to take another parameter for input on each device and change the device's address to match. Unfortunately, because addresses can include colons in them, the script will need to intelligently manage the input to only split by the first two colons.
Yes, forgot about that. Will update the PR.
@WasabiFan, I can try to add some advanced command line options parser (something like -d device -i index -a address
), or I can just parse options like touch_sensor:0@in1
(@in1
being optional). What do you think?
I'd vote for simple. Just pick a character (I like the @ idea) to use, and split by that just like the existing ones. Should be perfect for our needs.
So, that means I'm in favor of the latter option in your question.
Got that :)
54d3867 does that.
Looks good to me -- let's get a look over it from @rhempel so he knows what's happening and can make sure it looks good and then we can get this merged.
@ddemidov, is get_devices
solid? Would it be better called create_devices
or make_devices
?
Its function is to pull devices from a "device database" into testing arena. So may be pull_devices
?
populate_device_tree
?
test_devices
?
My thoughts:
create_devices
/ make_devices
It is creating device nodes, so this makes sense. But the device data is actually being copied from somewhere else, and the creation of the devices isn't what is important: the important bit is that there are now device nodes where you need them.
pull_devices
Makes sense to us, but for potential contributors, "pulling" devices isn't clear. It isn't very descriptive. Also see the comment above on the part of the action that is important.
test_devices
In its current state, it doesn't really test anything -- it does, however, do things with "test devices" so maybe we could prepend a verb there.
populate_device_tree
I like this one because it has a verb indicating what it is doing and a noun specifying what's changing: the device tree is being populated. I think this would make sense to someone trying to understand what this code does at a glance of the calling code.
It makes sense. I am sleeping right now, but I'll rename to populate_device_tree tomorrow if you don't come up with something less mouthful.
Yeah, it's pretty long. Maybe shorten it to populate_devices
. Whatever it is, I don't think it will make too much of a difference :wink:
populate_arena? goes with clean_arena
:+1: for populate-arena
populate_arena? goes with clean_arena
Sounds good
I've renamed the script and cleaned up the history.
This moves device nodes to
./devices
, and creates two helper scripts,get_devices.py
andclean_arena.py
, as discussed in #1.An example workflow:
pinging @WasabiFan