Chris McDonough <> 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 " nosetests" of the package after applying your 

Name                           Stmts   Exec  Cover   Missing
repoze.who                         0      0   100%   
repoze.who.classifiers            37     37   100%   
repoze.who.config                102    102   100%   
repoze.who.interfaces             17     17   100%   
repoze.who.middleware            255    255   100%   
repoze.who.plugins                 0      0   100%   
repoze.who.plugins.auth_tkt      104    104   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
repoze.who.plugins.form          132    132   100%   
repoze.who.plugins.htpasswd       45     45   100%   
repoze.who.plugins.sql           192    163    84%   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%   
TOTAL                            994    965    97%   

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 <>
Repoze-dev mailing list

Reply via email to