Bug#816192: RFS: python-proselint/0.3.5-1 [ITP] -- A prose linter
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
Reply to: