On Sat, 25 May 2013, Jakub Wilk wrote:
Welcome to DPMT!
Thanks!
I don't intend to sponsor this package, but here's my review:
I have a couple of DD friends I can bug to do the actual sponsored upload,
but review from folks with more knowledge of Python packaging and team
procedures is very useful, thanks.
Upstream provides documentation. It might be a good idea to build and ship
it.
Lintian says:
I: pygithub source: debian-watch-file-is-missing
Fixed, using githubredir.debian.net.
P: python-github: no-upstream-changelog
P: python3-github: no-upstream-changelog
There isn't one. I assume I shouldn't be creating one out of git shortlog
or something... although that does remind me that the upstream README.rst
wasn't getting installed, which is now fixed.
P: python-github: no-homepage-field
P: python3-github: no-homepage-field
Fixed.
I would drop the Provides fields:
http://lists.debian.org/debian-python/2011/03/msg00139.html
Oh, I see. Good to know. (That also makes moot my question of why
${python3:Provides} was not being set.)
Re override_dh_auto_install: -O0 is no-op when used together with
--no-compile. But then, I wouldn't use --no-compile, as is thwarts
possibility of spotting byte-compilation errors early.
I copied that from ScottK's packaging for python-ipaddr. I'm not sure why
it's there, but I can remove both of those since I don't think there's any
reason for them in this package.
Do tests require Internet connectivity? If no, then it would be good to run
them at build time.
Only one of them does (JSON encoding, github/tests/Issue142.py). I can
try to arrange to skip that one; the rest all pass under `unshare -n net`.
(There's some mechanism for mocking the API's responses.)
That said, I thought the eventual goal was to move tests to autopkgtest
instead of the build process? Are we sufficiently far away from that
reality that today I should still be putting this in the build process?
Typos in upstream code:
explicitely -> explicitly
instanciate -> instantiate
The code uses assert to validate types of method arguments. That's not what
assert is for.
I'll file issues / patches upstream about both of these, although no
promises about upstream agreeing with the latter stylistically. (These
should be converted into TypeErrors, right?)
Does pygithub validate SSL certificates?
Ugh. Good call, and I'm embarrassed not to have checked. Looks like it
uses httplib.HTTPSConnection, which doesn't. I will definitely come up
with a patch and file a pull request upstream before uploading!
Don't ignore errors from "rm -rf build". "-f" takes care of ENOENT and
you failures certainly should not go unnoticed.
Oh, good to know. I'd always seen people ignoring errors from rm in
Makefile clean targets, but your reasoning makes sense.
--
Geoffrey Thomas
http://ldpreload.com
geo...@ldpreload.com
--
To UNSUBSCRIBE, email to debian-python-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Archive:
http://lists.debian.org/alpine.deb.2.00.1305251554530.27...@dr-wily.mit.edu
