[Bug 14066] New: Koha code does not really use Readonly
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Bug ID: 14066 Summary: Koha code does not really use Readonly Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: ASSIGNED Severity: normal Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: jonathan.druart@biblibre.com Reporter: jonathan.druart@biblibre.com QA Contact: testopia@bugs.koha-community.org The Readonly and Readonly::XS package are only used in Koha::Calendar. IMO the added value of there packages does not justify their purpose. I would suggest to remove these packages as Koha deps. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #1 from Jonathan Druart <jonathan.druart@biblibre.com> --- Created attachment 38546 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38546&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |colin.campbell@ptfs-europe. | |com --- Comment #2 from Jonathan Druart <jonathan.druart@biblibre.com> --- Colin, do you have any concerns about this patch? -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 38546 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38546 Bug 14066: Remove the Readonly[::XS] dependency Review of attachment 38546: --> (http://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=14066&attachment=38546) ----------------------------------------------------------------- ::: 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 (=). -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Further reading. http://neilb.org/reviews/constants.html -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13767 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 38577 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38577&action=edit 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) 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38577|0 |1 is obsolete| | --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 38578 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38578&action=edit 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) 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38546|0 |1 is obsolete| | --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 38579 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38579&action=edit 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> Tested in conjuction with both patches. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38578|0 |1 is obsolete| | --- Comment #9 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 38580 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38580&action=edit 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) 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #10 from M. Tompsett <mtompset@hotmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch --- Comment #11 from Marc Véron <veron@veron.ch> --- 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... -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #12 from Marc Véron <veron@veron.ch> --- Created attachment 38591 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38591&action=edit [Signed-off] 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38579|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38591|0 |1 is obsolete| | --- Comment #13 from Marc Véron <veron@veron.ch> --- Comment on attachment 38591 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38591 [Signed-off] Bug 14066: Remove the Readonly[::XS] dependency Mistaked (once more...), sorry -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38546|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38546|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38579|1 |0 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #14 from Marc Véron <veron@veron.ch> --- Created attachment 38592 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38592&action=edit [Signed-off] 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38579|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #15 from Marc Véron <veron@veron.ch> --- Created attachment 38593 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38593&action=edit [Signed-off] 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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38580|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #38592|0 |1 is obsolete| | Attachment #38593|0 |1 is obsolete| | --- Comment #16 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 38787 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38787&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 38788 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38788&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Do we need a patch to remove from debian/control as well? I will attach a follow-up removing some more occurrences. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 38789 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=38789&action=edit Bug 14066: Removing more Readonly -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14066 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com Status|Passed QA |Pushed to Master --- Comment #20 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patches pushed to master. Thanks everyone! -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org