Chris McDonough chr...@plope.com added the comment:
Hi Douglas,
Sorry for not responding til now; the worthwhile patches always require more
thought than plain bugreports. Thanks for the submission!
(FTR, I tried to apply the patch but it has a syntax error on line 267. That
was easy to fix, just needed a tab char).
Anyway, it's a bit hard to tell from here: will this patch introduce any
backwards incompatibility when people upgrade to a version of who that contains
your patch?
If not (that's a bit of a deal-killer), a few things:
- To get this into who, we really need all the code covered by tests. Here's
output of a run of setup.py nosetests of the package after applying your
patch:
Name Stmts Exec Cover Missing
repoze.who 0 0 100%
repoze.who.classifiers37 37 100%
repoze.who.config102102 100%
repoze.who.interfaces 17 17 100%
repoze.who.middleware255255 100%
repoze.who.plugins 0 0 100%
repoze.who.plugins.auth_tkt 104104 100%
repoze.who.plugins.basicauth 45 45 100%
repoze.who.plugins.cookie 39 39 100%
repoze.who.plugins.fixtures coverage.CoverageException: No source for compiled
code
'/Users/chrism/projects/repoze/svn/repoze.who/trunk/repoze/who/plugins/fixtures/__init__.pyc'.
repoze.who.plugins.form 132132 100%
repoze.who.plugins.htpasswd 45 45 100%
repoze.who.plugins.sql 19216384% 25-26, 51, 65-76, 84-85,
88-92, 105-108, 128, 138-139, 165, 172, 201, 213-216, 224-225
repoze.who.restrict 23 23 100%
repoze.who.utils 3 3 100%
TOTAL99496597%
Without your patches, we get 100% code coverage, which is what we shoot for. A
lot of the un-covered code is likely due to conditional imports. But the
conditional imports themselves have a little bit of a smell, especially the ones
that try an import but then turn around and catch an ImportError exception and
reraise a different error to help. We usually prefer to allow original
exceptions to propagate up even if it means a slightly more cryptic error
message, it makes the code easier to read and maintain.
A few style things (picaynue but also important, to us at least):
- In default_password_compare, instead of putting things into a big
if/else/else, etc, can we use a dict full of functions by name and dispatch on
the name?
- The r.who source is formatted at 79 char linebreaks.
If fixing that stuff is too much work, I'd suggest forking the SQL plugin itself
into a separate package and releasing it by itself. That'd be a reasonable
thing either way, actually.
__
Repoze Bugs b...@bugs.repoze.org
http://bugs.repoze.org/issue85
__
___
Repoze-dev mailing list
Repoze-dev@lists.repoze.org
http://lists.repoze.org/listinfo/repoze-dev
