https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12029
Martin Renvoize <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #21 from Martin Renvoize <[email protected]> --- Welcome Bill, This looks like a great new feature and I'm really glad the discussion continued past comment 4 to get the refinements I also felt were necessary. Regarding the actual submission, did you read the coding guidelines available https://wiki.koha-community.org/wiki/Coding_Guidelines ? These are what the QA people will base their most basic feedback upon and are 'hard fails' in most cases when not adhered to. They are voted upon by a council of community members and have good reasoning behind each (though they do not cover all cases and being updated fairly regularly as we come across various patterns.. for exapme the 'filter_by_' requirement is not yet a formal guideline, but will become one very soon) Beyond hard fails, the QA persons role is to help spot regressions and ensure coding of a minimum standard is maintained for submissions so we can maintain the features going forward. Such advice can be argued against, but should generally be considered constructive. So, taking Jonathans list. 1. Style, this may fail guidelines given our perlcritic requirements. It's a 1s fix anyway. 2. Will be in the guidelines imminently, a few minutes to fix. 3. Style, improved coding style, readability improvement. Not a hard fail, but constructive. 4. Hard fail, all module changes have required corresponding unit tests since 2017. If you are stuck here, just ask for some guidance. https://wiki.koha-community.org/wiki/Coding_Guidelines#PERL17:_Unit_tests_are_required_.28updated_Apr_26.2C_2017.29 5. This is an unwritten rule.. I've made a note to add it to the guidelines. It just makes submissions considerably simpler for maintainers to add to all the supported branches. 6. Style, no guidelines broken, but it's a fair comment to help improve the developer. Another 10s fix. 7. Style, this will be fixed at push. 8. Hard fail: https://wiki.koha-community.org/wiki/Coding_Guidelines#Licence 9. Superfluous comments confuse future maintenance and cost everybody time. 10. Style, developers do things in their own way. Whilst I agree an ajax method would have been preferable (I'd probably have gone for a new REST route myself, rather than svc), this isn't a hard fail but more of a guide for future developments. 11. This is highlighting some red flags for future maintenance again. This is the most 'human' comment here. To me, this code looks rather 'copy/paste', which is fine but there should be evidence that you've stripped out all unnecessary boilerplate and given the development some thought as to what it's actually doing and requires. Hope that helps clarify things. -- 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/
