Bug#733489: python-apt: Improve 'Dependency' and 'BaseDependency' to get target package versions that satisfy dependencies
On 01/01/2014 05:44 PM, Michael Vogt wrote:
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.
You're welcome. I really like the idea to use Travis CI.
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!
Thanks. Could you include the detailed messages for the error codes?
(taken from --show-pep8)
E125 continuation line does not distinguish itself from next logical line
E126 continuation line over-indented for hanging indent
E127 continuation line over-indented for visual indent
E128 continuation line under-indented for visual indent
IMHO those should be fixed too because they increase the readability of
the source code. You can try to fix those with autopep8.
4) debian/control (nitpick)
I prefer to sort dependencies alphabetically.
I have no opinion about this :) I don't really mind either way.
Especially for packages with many dependencies I think it helps to check
if a dependency is already there or missing. But given that one doesn't
look often into debian/control I can really understand that there is no
strong opinion on it. ;-)
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.
Thanks for the good explanation. ^^
BTW: Don't you need a '-q' for 'sudo apt-get install build-essential
$(gdebi -q --apt-line ./debian/control)', too?
Cheers and happy new year!
Michael
Oh yeah. Happy new year!
Reply to: