https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #79 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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.