Koha::DateUtils::output_pref takes 4 parameters and the last one is a boolean, so some calls are: output_pref($dt, undef, undef, 1) Which is not readable.
Created attachment 18517 [details] [review] Bug 10380: Change prototype for output_pref routine Koha::DateUtils::output_pref takes 4 parameters and the last one is a boolean, so some calls are: output_pref($dt, undef, undef, 1) This patch changes it prototype to output_pref({ dt => $dt, dateformat => $dateformat, timeformat => $timeformat, dateonly => $boolean }); an alternative is to call the output_pref routine with a datetime object, without using an hashref: output_pref($dt); Test plan: Go on some page where dates are displayed (circulation, laterorder, member page, etc.). Launch the unit tests file t/DateUtils.t.
Created attachment 18597 [details] [review] [SIGNED-OFF] Bug 10380: Change prototype for output_pref routine Koha::DateUtils::output_pref takes 4 parameters and the last one is a boolean, so some calls are: output_pref($dt, undef, undef, 1) This patch changes it prototype to output_pref({ dt => $dt, dateformat => $dateformat, timeformat => $timeformat, dateonly => $boolean }); an alternative is to call the output_pref routine with a datetime object, without using an hashref: output_pref($dt); Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Created attachment 19188 [details] [review] Bug 10380: Change prototype for output_pref routine Koha::DateUtils::output_pref takes 4 parameters and the last one is a boolean, so some calls are: output_pref($dt, undef, undef, 1) This patch changes it prototype to output_pref({ dt => $dt, dateformat => $dateformat, timeformat => $timeformat, dateonly => $boolean }); an alternative is to call the output_pref routine with a datetime object, without using an hashref: output_pref($dt); Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
(In reply to comment #1) > This patch changes it prototype to > output_pref({ > dt => $dt, If we switch to named parameters, we may as well avoid over-abbreviating. Would there be any objection to a followup that renames 'dt' to 'date'?
Setting to "In discussion" pending the patch submitter answering the question I posed in comment 4.
(In reply to Galen Charlton from comment #5) > Setting to "In discussion" pending the patch submitter answering the > question I posed in comment 4. Oops sorry Galen, I did not see it! dt is an abbreviation for datetime, not date. So I don't think "date" is an appropriate name. However we could choose "datetime" instead.
Galen, should I switch this report to 'Passed QA'?
Created attachment 20906 [details] [review] Bug 10380: Followup Change prototype for output_pref routine Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Pushed to master, along with follow-ups to update a couple new calls to output_pref() and to improve the POD. Thanks, Jonathan!