Colin, do you have any concerns about this patch?

Comment on attachment 38546 [details] [review]
Bug 14066: Remove the Readonly[::XS] dependency

Review of attachment 38546 [details] [review]:
-----------------------------------------------------------------

::: C4/Installer/PerlDependencies.pm
@@ -240,5 @@
> -    'Readonly' => {
> -        'usage'    => 'Core',
> -        'required' => '1',
> -        'min_ver'  => '1.03'
> -    },

I think we should keep Readonly. It is one of the better ways of defining constants, and would allow us to improve our perlcritic levels in the future. Currently, we only require perlcritic -5.

@@ -245,5 @@
> -    'Readonly::XS' => {
> -        'usage'    => 'Core',
> -        'required' => '0',
> -        'min_ver'  => '1.02'
> -    },

However, from the Readonly page on CPAN:
"I repeat, you do not need Readonly::XS if your environment has perl 5.8.x or higher."
Perhaps just removing Readonly::XS is the better solution. :)

::: Koha/Calendar.pm
@@ +126,4 @@
>  
>      if ( $unit eq 'hours' ) {
>          # Fixed for legacy support. Should be set as a branch parameter
> +        my $return_by_hour => 10;

Readonly's definition of a scalar value is confusing, such that when you changed it to a simple variable to accidentally left what looks like hashiness (=>), even though it probably should be plainly scalar (=).

Some interesting reading while researching making things read only.

"The interface for this module was inspired by Eric Roode's Readonly. The implementation is inspired by doing everything the opposite way Readonly does it."
(http://search.cpan.org/~leont/Const-Fast-0.014/lib/Const/Fast.pm)

Further reading.
http://neilb.org/reviews/constants.html

P.S. I did test around in Home->Tools->Calendar too, and attempting to look for sunday elsewhere in code.

P.P.S. It's beautiful to have a white Perl modules tab.

Testing with both patches, following test plan:

1) Apply both patches

OK

2) prove -v t/db_dependent/Calendar.t

?? t/db_dependent/Calendar.t
This file does not exist.
find -name Cal*.t
Result: ./t/Calendar.t
OK - error in test plan :-)

3) prove -v t/db_dependent/Holidays.t
OK

4) prove -v t/Calendar.t
OK

5) perlcritic -3 Koha/Calendar.pm
   -- the message corresponds to a comment in the code already
      and not this change.
OK, one message found: 
Package variable declared or used at line 60, column 1.  See pages 73,75 of PBP.  (Severity: 3)

6) run koha qa test tools
qa -c 2 [-v 2]
OK	Koha/Calendar.pm
OK	C4/Installer/PerlDependencies.pm

Will sign off...

Comment on attachment 38591 [details] [review]
[Signed-off] Bug 14066: Remove the Readonly[::XS] dependency

Mistaked (once more...), sorry

Created attachment 38787 [details] [review]
[PASSED QA] Bug 14066: Remove the Readonly[::XS] dependency

The Readonly and Readonly::XS package are only used in Koha::Calendar.
IMO the added value of there packages does not justify their purpose.

Test plan:
1/ Remove Readonly and Readonly::XS
2/ Open the calendar and confirm you don't get any error concerning missing
dependency

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Created attachment 38788 [details] [review]
[PASSED QA] Bug 14066: Correct and optimize

As far as I can tell $sunday is never used. And it seemed to be
scoped to only _init.

The remaining hashiness was cleaned up for the $return_by_hour.

TEST PLAN
---------
1) Apply both patches
2) (Skip this one - file does not exist / MV) prove -v t/db_dependent/Calendar.t
3) prove -v t/db_dependent/Holidays.t
4) prove -v t/Calendar.t
5) perlcritic -3 Koha/Calendar.pm
   -- the message corresponds to a comment in the code already
      and not this change.
6) run koha qa test tools

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Do we need a patch to remove from debian/control as well? 

I will attach a follow-up removing some more occurrences.

Created attachment 38789 [details] [review]
Bug 14066: Removing more Readonly

Patches pushed to master.

Thanks everyone!