On Sun, Feb 28, 2016 at 11:53 PM, Víctor Cuadrado Juan wrote: > I am looking for a sponsor for my package "python-proselint" ... > Proselint aggregates knowledge about best practices in writing from world's > greatest writers and editors, and makes it accessible by giving suggestions in > the form of a linter for prose.
This sounds useful, thanks for packaging it. Here is a review: I can't find a copy of the BSD license grant in the upstream tarball at all, only in PKG-INFO and setup.py. I'm not sure this is good enough for ftp-master approval. Please talk to upstream about including a license grant in a README or in setup.py or something. They have a LICENSE.md file in their github repository so they probably just forgot to include it and README.md and other files in the source distribution. The upstream test system assumes proselint is installed instead of testing the code in the source tree. The proselint command when installed prints a lot of exceptions. It would be great if upstream would sign their releases using OpenPGP. https://wiki.debian.org/debian/watch#Cryptographic_signature_verification https://help.riseup.net/en/security/message-security/openpgp/best-practices I'm not sure you need separate Python 2 and Python 3 packages. Are there any users of the proselint Python module apart from the proselint command-line tool? If not I would just create a proselint binary package that uses Python 3. Either way you definitely do not need proselint and proselint3 command-line tools, since they offer identical functionality. If you decide on a proselint package, it should be Section: text, not Section: python. I would suggest to forward the manual page upstream. If they want to have the manual page automatically generated, the options I know of include sphinx and sphinxcontrib-autoprogram or python3-sphinx-argparse. Upstream may want to add shell completion, which can be done automatically using python3-argcomplete. You may want to wrap and sort the debian/ packaging to make diffs easier to read. I use this: wrap-and-sort --short-indent --wrap-always --sort-binary-packages --trailing-comma --verbose I'm seeing a disturbing amount of calls to subprocess functions with shell=True or where equivalent Python functions could be used, please send upstream a patch to avoid using subprocess or call functions from it without using shell=True. Forking subprocesses is more expensive than handling it in Python and using shell=True can be potentially dangerous so it is a bad habit to get into even if it is safe where used in proselint (if it is not, they need to issue some security advisories). The Process Identifier Preservation Society will thank you! http://bonedaddy.net/pabs3/log/2014/02/17/pid-preservation-society/ http://oss-security.openwall.org/wiki/disclosure/cve No need for shell=True: out = subprocess.check_output("proselint --version", shell=True) subprocess.call("proselint --debug >/dev/null", shell=True) Potentially dangerous depending on the arguments: out = subprocess.check_output("proselint {}".format(fullpath), shell=True) subprocess.call("{} {}".format("open", fullpath), shell=True) subprocess.call("proselint {} >/dev/null".format(filepath), shell=True) Use the python equivalents instead: subprocess.call("find . -name '*.pyc' -delete", shell=True) subprocess.call("rm -rfv proselint/cache > /dev/null && mkdir -p {}".format(os.path.join(os.path.expanduser("~"), ".proselint")), shell=True) Please add some upstream metadata: https://wiki.debian.org/UpstreamMetadata Automatic checks: build: I: pybuild base:184: cd /build/python-proselint-0.3.5/.pybuild/pythonX.Y_2.7/build; python2.7 -m unittest discover -v /bin/sh: 1: proselint: not found [(1, 15, 'garner.dates', u"When specifying a date range, write 'from X to Y'.")] Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored Exception TypeError: "'NoneType' object is not callable" in ignored I: pybuild base:184: cd /build/python-proselint-0.3.5/.pybuild/pythonX.Y_3.5/build; python3.5 -m unittest discover -v /bin/sh: 1: proselint: not found lintian: P: python-proselint source: debian-watch-may-check-gpg-signature X: python3-proselint: library-package-name-for-application usr/bin/proselint3 X: python3-proselint: application-in-library-section python usr/bin/proselint3 P: python3-proselint: no-upstream-changelog X: python-proselint: library-package-name-for-application usr/bin/proselint X: python-proselint: application-in-library-section python usr/bin/proselint P: python-proselint: no-upstream-changelog check-all-the-things: $ cme check dpkg ... Warning in 'control source Build-Depends:2' value 'python-all (>= 2.6.6-3~)': unnecessary versioned dependency: python-all >= 2.6.6-3~. Debian has squeeze -> 2.6.6-3+squeeze7; wheezy -> 2.7.3-4+deb7u1; jessie-kfreebsd -> 2.7.9-1; jessie -> 2.7.9-1; stretch -> 2.7.11-1; sid -> 2.7.11-1; $ codespell --quiet-level=3 ./proselint/command_line.py:148: intialization ==> initialization <more, not sure if they are deliberate or not> # check if these can be switched to https:// $ grep -rF http: . <lots> $ grep -riE 'fixme|todo|hack|xxx|broken' . ./proselint/checks/garner/needless_variants.py: # ["password", ["passcode"]], # FIXME ./proselint/checks/wallace/uncomparables.py: ("more", "possible") # FIXME # $ find -type d \( -iname .bzr -o -iname .git -o -iname .hg -o -iname .svn -o -iname CVS -o -iname RCS -o -iname SCCS -o -iname _MTN -o -iname _darcs -o -iname .pc -o -iname .cabal-sandbox -o -iname .cdv -o -iname .metadata -o -iname CMakeFiles -o -iname _build -o -iname _sgbak -o -iname autom4te.cache -o -iname blib -o -iname cover_db -o -iname node_modules -o -iname '~.dep' -o -iname '~.dot' -o -iname '~.nib' -o -iname '~.plst' \) -prune -o -type f ! \( -iname '*.bak' -o -iname '*.swp' -o -iname '#.*' -o -iname '#*#' -o -iname 'core.*' -o -iname '*~' -o -iname '*.gif' -o -iname '*.jpg' -o -iname '*.jpeg' -o -iname '*.png' -o -iname '*.min.js' -o -iname '*.js.map' -o -iname '*.js.min' -o -iname '*.min.css' -o -iname '*.css.map' -o -iname '*.css.min' \) -exec spellintian --picky {} + ./tests/test_garner_dates.py: uneeded -> unneeded ./proselint/demo.md: preceeding -> preceding ./proselint/demo.md: latin -> Latin ./proselint/checks/pinker/metadiscourse.py: preceeding -> preceding ./proselint/command_line.py: intialization -> initialization $ find -type f -iname '*.py' -exec pylint --msg-template='{path}:{line}:{column}: [{category}:{symbol}] {obj}: {msg}' --reports=n {} + <lots> $ find -type f -iname '*.py' -exec pep8 --ignore W191 {} + <lots more of these 3 warnings> ./proselint/checks/norris/denizen_labels.py:39:23: E241 multiple spaces after ',' ./proselint/tools.py:211:41: E226 missing whitespace around arithmetic operator ./proselint/tools.py:211:46: E226 missing whitespace around arithmetic operator -- bye, pabs https://wiki.debian.org/PaulWise
