On Wed, Jan 01, 2014 at 12:30:16PM +0100, Michael Schaller wrote: > A few comments on the patches:
Thanks for your review and the points you raised. > 1) tests/test_pep8.py > Can you document in a comment or docstring why you ignore certain > issues and what these issues are? This should help readers to better > understand what the test does and more importantly what it ignores. > Furthermore do you plan to reduce the number of ignores in the > future? If so you should add a TODO for yourself. > > 2) tests/test_pep8.py > Can you use instead of 'self.assertEqual(res, 0)' a better fail > message like you did in 'tests/test_pyflakes.py'? > > 3) tests/test_pyflakes.py > You don't need the 'from __future__ import print_function' import > with Python 3. Those are fixed in my git branch now, thanks! > 4) debian/control (nitpick) > I prefer to sort dependencies alphabetically. I have no opinion about this :) I don't really mind either way. > 5) .travis.yml > I would really like to see a rather lengthy comment in the beginning > of the YAML file to explain what purpose it serves and what it does. > I also think you should give a link here and that you should explain > what should happen after Trusty. Maybe it would be even better to > use SID instead of Trusty. > > 6) .travis.yml > Why do you run './debian/rules binary' and not 'dpkg-buildpackage'? > > 7) .travis.yml > Why not also ensure here that Lintian and if possible piuparts have > no issues with the newly built package? Good points, I added a explaination to the top now that hopefully addresses the points. I renamed the added sources.list entry as its misleading. In my branch its using "distro-info --stable" to get test latest python3.3 and apt/libapt. Using sid here is probably a bit too fragile, the chroots of the travis-ci.org are ubuntu based. But I would certainly love to see a travis-ci instance that is debian based. I changed the "debian/rules binary" in my branch now to be "./debian/rules build-arch". This will build and run the tests. We can't use dpkg-buildpackage currently as its using fakeroot and the tests (iirc in the test_auth.py code) also use fakeroot and nesting is not possible AFAIK. But I do like the idea of building the deb and doing additional checks like lintian/piuparts. Cheers and happy new year! Michael -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
