I've noticed a number of Perl modules have shebangs in them. Unless there is a good reason for them to be there, they should be removed. Examples: C4/Barcodes/ValueBuilder.pm C4/SIP/Logger.pm C4/SIP/SIPServer.pm C4/ShelfBrowser.pm Koha/Z3950Responder.pm Koha/Z3950Responder/GenericSession.pm Koha/Z3950Responder/RPN.pm Koha/Z3950Responder/Session.pm Koha/Z3950Responder/ZebraSession.pm
Created attachment 111584 [details] [review] Bug 26673: Remove shebangs from Perl modules It's not needed.
FAIL C4/Barcodes/ValueBuilder.pm FAIL critic # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).
(In reply to Jonathan Druart from comment #2) > FAIL C4/Barcodes/ValueBuilder.pm > > FAIL critic > > # Modules::RequireFilenameMatchesPackage: Got 1 violation(s). Not caused by this change though, maybe you want to file separate bug for this and the same issue with Koha/Z3950Responder/RPN.pm, and actually not totally sure these are bugs.
FAIL Koha/Z3950Responder/RPN.pm FAIL critic # Modules::RequireFilenameMatchesPackage: Got 1 violation(s).
This patch missed one instance where the shebang is not needed: C4/TTParser.pm
Created attachment 111655 [details] [review] Bug 26673: Remove shebangs from Perl modules It's not needed. Signed-off-by: David Cook <dcook@prosentient.com.au>
Created attachment 111656 [details] [review] Bug 26673: Remove #!/usr/bin/env perl from .pm modules too
(In reply to Joonas Kylmälä from comment #5) > This patch missed one instance where the shebang is not needed: > C4/TTParser.pm Nice catch. I've added a patch for that.
(In reply to Joonas Kylmälä from comment #3) > Not caused by this change though, maybe you want to file separate bug for > this and the same issue with Koha/Z3950Responder/RPN.pm, and actually not > totally sure these are bugs. Yeah, I don't think we should fail this bug for those Perl critic failures. I'll open a new bug report for those though. In theory, C4/Barcodes/ValueBuilder.pm and Koha/Z3950Responder/RPN.pm would just become packages that "use" the subpackages that can be split off into their own files. (I'll take a look if there are any other packages that are guilty of the same Perl critic violation...)
(In reply to David Cook from comment #9) > I'll open a new bug report for those though. In theory, > C4/Barcodes/ValueBuilder.pm and Koha/Z3950Responder/RPN.pm would just become > packages that "use" the subpackages that can be split off into their own > files. (I'll take a look if there are any other packages that are guilty of > the same Perl critic violation...) Ok C4/Barcodes/ValueBuilder.pm was easy, but I just realized Koha/Z3950Responder/RPN.pm overrides a non-C4/non-Koha namespace. Blah. This is where our terrible codetree layout comes to bite us heh.
Ok I've opened Bug 26683 to deal with the Perl critic issues
Created attachment 111677 [details] [review] Bug 26673: Remove shebangs from Perl modules It's not needed. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 111678 [details] [review] Bug 26673: Remove #!/usr/bin/env perl from .pm modules too Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 111810 [details] [review] Bug 26673: Remove #!/usr/bin/env perl from .pm modules too Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111811 [details] [review] Bug 26673: Remove shebangs from Perl modules It's not needed. Signed-off-by: David Cook <dcook@prosentient.com.au> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111812 [details] [review] Bug 26673: Remove #!/usr/bin/env perl from .pm modules too Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Could we have a QA script check for this?
(In reply to Katrin Fischer from comment #17) > Could we have a QA script check for this? Oooh that's a good idea!
Pushed to master for 20.11, thanks to everybody involved!
David, this change makes t/00-testcritic.t fail on # Failed test 'Test::Perl::Critic for "C4/Barcodes/ValueBuilder.pm"' # at /usr/share/perl5/Test/Perl/Critic.pm line 121. # # Package declaration must match filename at line 19, column 1. Correct the filename or package statement. (Severity: 5) # Failed test 'Test::Perl::Critic for "Koha/Z3950Responder/RPN.pm"' # at /usr/share/perl5/Test/Perl/Critic.pm line 121. # # Package declaration must match filename at line 1, column 1. Correct the filename or package statement. (Severity: 5) I am aware of bug 26683 but I'd like a quick fix to have the tests green now.
Created attachment 112814 [details] [review] Bug 26673: Fix perlcritic complains This patch fixes the following perlcritic complain: Package declaration must match filename at line 19, column 1. Correct the filename or package statement.
(In reply to Jonathan Druart from comment #21) > Created attachment 112814 [details] [review] [review] > Bug 26673: Fix perlcritic complains > > This patch fixes the following perlcritic complain: > Package declaration must match filename at line 19, column 1. Correct the > filename or package statement. Pushed to master for 20.11
backported to 20.05.x for 20.05.06
enhancement, not backported to 19.11.x