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.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 code '/Users/chrism/projects/repoze/svn/repoze.who/trunk/repoze/who/plugins/fixtures/__init__.pyc'. 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 <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
