http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7067
Jared Camins-Esakov <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #123 from Jared Camins-Esakov <[email protected]> --- There are no unit tests for Koha::Borrower::Modifications, nor for the AddMember_Opac routine (any other routines added should also have unit tests, of course). Starting with the 3.12 release cycle, unit tests are required for code added to the Koha:: and C4:: namespaces. Also, please use hashrefs rather than hashes as arguments in the Koha:: namespace. I could have sworn you did a follow-up changing all the hash arguments to hashrefs but I don't see it anywhere, even among the obsolete patches. Other notes: * Do not access the database in BEGIN {} blocks, especially not in the Koha:: namespace. * Use of C4::SQLHelper from the Koha:: namespace. Calling into the C4:: namespace from Koha:: is not supposed to be done. If that was the only objection, I would probably push it anyway, at least this time, but arguably I probably shouldn't. * Package-level my variables are verboten, since they break persistence, and replacing "my" with "our" should be done only under extreme duress, and never in new code (note: you can use our when it's called for by the code, just not as a workaround for my not working under Plack). Thank you for including a test plan on the bug. I copied it (and the original RFC) into the commit message for the first patch. I did not review it for accuracy yet, since I discovered the lack of unit tests before I got that far, so you may want to do that. Also, I noticed a few other issues that would not prevent me pushing this but you might want to keep in mind for the future: * The standard for help in command-line scripts is to use pod2usage. * Object-oriented classes do not export routines and therefore should not use Exporter. Even procedural classes that do not export any routines should not use Exporter. * When creating ->new() subroutines, the following idiom may be useful: return bless( { 'verification_token' => $args{'verification_token'}, ... }, $class ); Or even: return bless( $args, $class ); I find those two idioms make code easier to read, and certainly save typing, but they are by no means required. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs website : http://www.koha-community.org/ git : http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
