[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

Bug#710103: RFS: evdev/0.3.2-1 [ITP] -- python bindings for the generic input event interface



Hi,

Thanks for the review!

Most of the upstream source issues have been looked into by the upstream
developer (Georgi Valkov) and probably already fixed. I am looking into
the packaging fixes right now.

We will be also switching to the python-evdev name. I will update the
sources once more, there will be a new upstream version coming out with
the fixes as well. So most of the things will be fixed afterwards.

Once again thanks for all the comments!

Best regards,

On 29.05.2013 00:05, Jakub Wilk wrote:
> I don't intend to sponsor this package, but here's my review:
> 
> * Łukasz 'sil2100' Zemczak <lukasz.zemczak@canonical.com>, 2013-05-28,
> 12:04:
>> http://mentors.debian.net/debian/pool/main/e/evdev/evdev_0.3.2-1.dsc
> 
> "evdev" as source package name sounds a bit too generic. Maybe make it
> "python-evdev"? Apparently even upstream uses the latter name.
> 
> "debhelper (>= 9.0.0)" - debhelper no longer uses such versioning
> scheme. Use just "debhelper (>= 9)".
> 
> Vcs-* fields are supposed to point to Debian packaging repository, not
> to upstream one.
> 
> PKG-INFO says "Classifier: Programming Language :: Python :: 3.1" but
> debian/control says "X-Python3-Version: >= 3.2". Which one is correct?
> 
> Lintian says:
> I: evdev source: duplicate-short-description python-evdev python3-evdev
> I: evdev source: quilt-patch-missing-description include_license.patch
> P: python-evdev: no-upstream-changelog
> P: python3-evdev: no-upstream-changelog
> 
> (You could easily extract changelog from README.)
> 
> "rm -f" already ignores ENOENT, and you don't want to ignore any other
> errors. Don't prefix "rm" with "-".
> 
> Typo in evdev.device.InputDevice.capabilities docstring:
> as a a mapping -> as a mapping
> 
> In the get_sw_led_snd() function:
> - "max" could be uninitialized;
> - the buffer size is too small; it should be (max+7)/8.
> - the code doesn't check if malloc(3) returns NULL;
> - PyMem_Malloc()/PyMem_Free() should probably be used instead of
> malloc(3)/free(3); see: <http://docs.python.org/2/c-api/memory.html>.
> 
> But on a second thought, this dynamic allocation is not necessary at
> all. LED_MAX, SW_MAX and SND_MAX are all small compile-time constants,
> so you could just declare an appropriately-sized array as an automatic
> variable.
> 
> device_read() returns None if the underlying read(2) call fails. That's
> not very pythonic; I'd expect that it throws IOError in such situations.
> 
> device_read_many() is similar in this regard.
> 
> ioctl_capabilities() and get_sw_led_snd() don't check return value of
> ioctl(2).
> 
> "except" without exception type is a bad idea:
> http://docs.python.org/2/howto/doanddont.html#except
> 
> __all__ is supposed to contain only strings.
> 


-- 
Łukasz 'sil2100' Zemczak
 lukasz.zemczak@canonical.com
 www.canonical.com
 www.ubuntu.com


Reply to: