Code review comment for lp:~elopio/autopilot/no_uinput_side-effects

Revision history for this message
Christopher Lee (veebers) wrote :

Hi Leo, thanks for making those changes. I only have a couple more :-)

Most important is line: 302 will raise an exception as _get_touch_events isn't
passed anything (I presume res_x/res_y are the intended vars.)

line 213: It is possible (not likely, but possible) that cls._device will None,
please make a check here.
(The on_test_end stuff should be going away in the near future as it's a little
hacky, but is here to stay for now.)

Can you please add a test for this and make the fix? Also, if you could run
this over the Unity8 tests to ensure there are no other regressions (due to
there not being 100% coverage of this module).

Line 249: Can you expand on the deprecated message please? Something like "the
Touch class to instantiate a device object" or similar?
(Currently the deprecated method outputs this string: "This function is
deprecated. Please use '%(your_string_here)s' instead.")

lines 401 - 404: The docstring appears to contain details intended for the
__init__ method.

line 410: Minor: It seems inconstant to me to have this number stored as a
class variable where previously (i.e. press_value line 975) has been used
locally. I'm not really suggesting a fix, just noticing it.

line 547-548: raises details: get_center_point can also raise exceptions,
either add them to the docstring or easier, add a mention re: get_center_point
and it's docs.

574,586: the word 'touch' should probably be '"finger"' in the docstring
(":raises RuntimeError: if the touch is not pressed." )

review: Needs Fixing

« Back to merge proposal