https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25408 --- Comment #93 from Arthur Suzuki <arthur.suzuki@biblibre.com> --- (In reply to Victor Grousset/tuxayo from comment #90)
- Unnecessary changes are made e.g. $patron->borrowernumber to $patron->id (correct me if I'm wrong here, please!)
Indeed, in bunch of lines that are already changed, it's a good opportunity to enforce terminology without adding more conflict possibilities. But here it's in lines that are not changed, so it's adding new conflicts possibilities with other submitted patches and when backporting.
I've fixed this in the latest version of the patch :)
- Some lines that have not been changed have been tidied and included
Yes, also that.
Also fixed :)
- Personally I'd prefer this to be unfolded in more commits to ease the review process, with each having a short explanation of why the changes are required:
At least having the full commit message list these points. Not sure about splitting the commit after the work is done. Maybe I missed the main point, but in this case the 3 changes look 95% likely to have been done together and not as iteration with a different logic on each step. That's a nice thing to keep in mind for other reviews or my own patches, thanks :)
I've added the relevant points to the commit message, next time I will try to better split the commits. I don't know if the patch can move to "Passed QA" or if further work is needed? Thanks for the review anyway :) Best, Arthur -- You are receiving this mail because: You are watching all bug changes.