Bug 10380 - output_pref should use a hashref for parameters
Summary: output_pref should use a hashref for parameters
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-30 12:37 UTC by Jonathan Druart
Modified: 2014-12-07 20:02 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 10380: Change prototype for output_pref routine (9.86 KB, patch)
2013-05-30 12:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10380: Change prototype for output_pref routine (9.77 KB, patch)
2013-06-03 03:40 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 10380: Change prototype for output_pref routine (9.82 KB, patch)
2013-06-21 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10380: Followup Change prototype for output_pref routine (2.14 KB, patch)
2013-09-09 11:27 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-05-30 12:37:46 UTC
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.
Comment 1 Jonathan Druart 2013-05-30 12:42:22 UTC Comment hidden (obsolete)
Comment 2 Srdjan Jankovic 2013-06-03 03:40:24 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-06-21 13:25:28 UTC
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>
Comment 4 Galen Charlton 2013-06-21 14:31:14 UTC
(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'?
Comment 5 Galen Charlton 2013-07-02 13:33:25 UTC
Setting to "In discussion" pending the patch submitter answering the question I posed in comment 4.
Comment 6 Jonathan Druart 2013-07-02 13:45:13 UTC
(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.
Comment 7 Jonathan Druart 2013-09-06 07:22:06 UTC
Galen, should I switch this report to 'Passed QA'?
Comment 8 Jonathan Druart 2013-09-09 11:27:05 UTC
Created attachment 20906 [details] [review]
Bug 10380: Followup Change prototype for output_pref routine

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 9 Galen Charlton 2013-10-17 23:42:15 UTC
Pushed to master, along with follow-ups to update a couple new calls to output_pref() and to improve the POD.

Thanks, Jonathan!