https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608
Tomás Cohen Arazi <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #79 from Tomás Cohen Arazi <[email protected]> --- I generally like the cleanup being made in the area. Great job! I have some QA remarks that I consider blockers. Sorted randomly. 1) Changes to POD in Koha::Statistic are wrong in terms of consistency: -=head2 Class methods +=head2 METHODS ... -=head2 Internal methods +=head2 INTERNAL METHODS Please keep the 'style' we picked some years ago. 2) Koha::Statistic->_key_or_default feels bad. I'd prefer a ternary operator. 3) Koha::Statistic->insert means introducing a new pattern, and I don't think we need it. I'd ask you to file a new bug for proposing that pattern and have devs discuss it there, so this enhancement is not blocked. 4) C4/Stats.pm => +use Koha::Statistic; ^^ it should use the plural class. 5) Probably for Martin's follow-up: if we are tidying an entire hash structure, I usually suggest we sort keys on the same run. I can do it inline, but thought it was worth mentioning explicitly. 6) This would benefit from some patch squashing. I'm happy to push this ASAP once the issues are solved. -- You are receiving this mail because: You are watching all bug changes. _______________________________________________ Koha-bugs mailing list [email protected] https://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/
