I also noticed there's no rule forbidding subroutine definitions in PL

I think Tomas is going to add it to the agenda, as it corresponds with many other rules, especially those relating to object oriented code and unit tests. Yes, I imagine there should be a rule for being careful with the scoping of variables as well, especially in regard to Plack (and mod_perl). David Cook Systems Librarian Prosentient Systems 72/330 Wattle St, Ultimo, NSW 2007 From: Marcel de Rooy [mailto:M.de.Rooy@rijksmuseum.nl] Sent: Tuesday, 29 September 2015 7:34 PM To: David Cook <dcook@prosentient.com.au>; koha-devel@lists.koha-community.org Subject: RE: [Koha-devel] Add rule for no subroutines in PL scripts scripts. I thought that we had a rule against > that as well. We don't have it. I would rather not have it too. There is a problem with Plack if you define lexical variables at top level and refer to them in the subroutines. Should be a rule for that?

The alternative would be to add some code extremely specific to some script (like parsing some line of data) into a Koha/ library? Seems more messy. I like to isolate my mess. Philippe Blouin, Responsable du développement informatique Tél. : (888) 604-2627 philippe.blouin@inLibro.com <mailto:philippe.blouin@inLibro.com> inLibro | pour esprit libre | www.inLibro.com <http://www.inLibro.com> On 09/29/2015 09:24 AM, Tomas Cohen Arazi wrote:

2015-09-29 9:23 GMT-03:00 Philippe Blouin <philippe.blouin@inlibro.com <mailto:philippe.blouin@inlibro.com>>:

Hi! Morning here, maybe my brain is not ignited yet. But why such a rule? (if i understand its meaning correctly).

Because that way there's no way to properly write unit tests for the function. And code gets messy too. I'll call for a dev meeting and add this to the agenda.

-- Tomás Cohen Arazi Theke Solutions (http://theke.io <http://theke.io/>) ✆ +54 9351 3513384 GPG: B76C 6E7C 2D80 551A C765 E225 0A27 2EA1 B2F3 C15F

Because the code would be extremely specific to a specific functionality, whereas the code in the libraries is there to be reused. And let not go overboard with OO. A script is a script is a script. Make it readable, add functions to make it cleaner, make your functions readable. And make some rule for forbidding direct DB accesses, so that future DB changes do not require to change many scripts. But please, do not forbid functions in scripts. As for tests, writing them for very complicated, very very specific function only applicable to a script could have value, but it certainly does not balance the extra complexity your bringing to the code. Philippe Blouin, Responsable du développement informatique Tél. : (888) 604-2627 philippe.blouin@inLibro.com <mailto:philippe.blouin@inLibro.com> inLibro | pour esprit libre | www.inLibro.com <http://www.inLibro.com> On 09/29/2015 07:35 PM, David Cook wrote:

Philippe, could you explain how it’s more messy to have code in modules?

Generally speaking, I can’t think of a reason why there would be code that is extremely specific to some script. I think the preference these days is to use object oriented code, which does isolate mess, so the code should be in the class.

As Tomas points out, it also makes it more difficult to write unit tests for functions, if it’s isolated in a script with no other way of calling that code.

David Cook

Systems Librarian

Prosentient Systems

72/330 Wattle St, Ultimo, NSW 2007

*From:*Philippe Blouin [mailto:philippe.blouin@inlibro.com] *Sent:* Wednesday, 30 September 2015 12:52 AM *To:* Tomas Cohen Arazi <tomascohen@gmail.com> *Cc:* David Cook <dcook@prosentient.com.au>; koha-devel@lists.koha-community.org *Subject:* Re: [Koha-devel] Add rule for no subroutines in PL scripts

The alternative would be to add some code extremely specific to some script (like parsing some line of data) into a Koha/ library? Seems more messy.

I like to isolate my mess.

Philippe Blouin, Responsable du développement informatique

Tél. : (888) 604-2627 philippe.blouin@inLibro.com <mailto:philippe.blouin@inLibro.com>

inLibro| pour esprit libre |www.inLibro.com <http://www.inLibro.com>

On 09/29/2015 09:24 AM, Tomas Cohen Arazi wrote:

2015-09-29 9:23 GMT-03:00 Philippe Blouin <philippe.blouin@inlibro.com <mailto:philippe.blouin@inlibro.com>>:

Hi! Morning here, maybe my brain is not ignited yet. But why such a rule? (if i understand its meaning correctly).

Because that way there's no way to properly write unit tests for the function. And code gets messy too.

I'll call for a dev meeting and add this to the agenda.

--

Tomás Cohen Arazi

Theke Solutions (http://theke.io <http://theke.io/>) ✆+54 9351 3513384 GPG: B76C 6E7C 2D80 551A C765 E225 0A27 2EA1 B2F3 C15F

On Wed, Sep 30, 2015 at 10:18 AM, Tomas Cohen Arazi <tomascohen@gmail.com> wrote:

2015-09-30 9:20 GMT-03:00 Philippe Blouin <philippe.blouin@inlibro.com>:

...

Because the code would be extremely specific to a specific functionality, whereas the code in the libraries is there to be reused. And let not go overboard with OO.

We embrace the OO paradigm for most of the CRUD operations, but we also have libs containing functions that are used statically, like Koha::Util::MARC, that do specific things and are testable. And is only used in a couple places.

...

A script is a script is a script. Make it readable, add functions to make it cleaner, make your functions readable.

We have two main kind of scripts: command line scripts, and controller scripts that got too complicated during Koha's life, and we are trying to simplify "as we go".

I think both should be testable.

I would like to make a plea here from a support / trouble-shooting perspective: The controller scripts are hard to trouble-shoot, because they're CGI -- it's theoretically possible to run the perl debugger across them, but that requires a fair amount of mucking with the environment... If I'm trying to figure out what's happening with some chunk of code, I'd like to write a command line wrapper around a library, and trigger the bits that I want in a controlled manner. I can't do that with any of the code inside the controllers. Keeping the code in the controllers thin is important not only for test-ability from a unit testing perspective, but also from a trouble-shooting perspective, because they end up being black-boxes that only run inside of Apache. --Barton

Thanks for that Tomas. I don’t think I have anything substantive to add. Recently, I’ve done a lot of work on another Perl project which often doesn’t use subroutines or has them scattered around in files, which make them impossible to test. It makes development more difficult and it makes releases a nightmare. As Tomas says, you need to be able to test everything. If you think about it in that way, no code is every used once. It always has to be used at least twice. David Cook Systems Librarian Prosentient Systems 72/330 Wattle St, Ultimo, NSW 2007 From: Tomas Cohen Arazi [mailto:tomascohen@gmail.com] Sent: Thursday, 1 October 2015 12:19 AM To: Philippe Blouin <philippe.blouin@inlibro.com> Cc: David Cook <dcook@prosentient.com.au>; koha-devel <koha-devel@lists.koha-community.org> Subject: Re: [Koha-devel] Add rule for no subroutines in PL scripts 2015-09-30 9:20 GMT-03:00 Philippe Blouin <philippe.blouin@inlibro.com <mailto:philippe.blouin@inlibro.com> >: Because the code would be extremely specific to a specific functionality, whereas the code in the libraries is there to be reused. And let not go overboard with OO. We embrace the OO paradigm for most of the CRUD operations, but we also have libs containing functions that are used statically, like Koha::Util::MARC, that do specific things and are testable. And is only used in a couple places. A script is a script is a script. Make it readable, add functions to make it cleaner, make your functions readable. We have two main kind of scripts: command line scripts, and controller scripts that got too complicated during Koha's life, and we are trying to simplify "as we go". I think both should be testable. And make some rule for forbidding direct DB accesses, so that future DB changes do not require to change many scripts. But please, do not forbid functions in scripts. As for tests, writing them for very complicated, very very specific function only applicable to a script could have value, but it certainly does not balance the extra complexity your bringing to the code. I'm trying to think of a use case for that, and I'm not sure we really have that kind of situations. We might consider on a per-scenario basis BUT, the whole point is that no super-complex code should lack full coverage tests. Because the more complex the function, the more difficult for people to know they are not breaking a feature if they touch it. So, even if we didn't write this rule down, the most probable scenario is that the release team would fail a patch adding complex functions that don't provide unit tests. This is not a religious/fanatic thing, but a practical one. Trying to pay our technical debt and interests instead of adding more debt. -- Tomás Cohen Arazi Theke Solutions (http://theke.io <http://theke.io/> ) ✆ +54 9351 3513384 GPG: B76C 6E7C 2D80 551A C765 E225 0A27 2EA1 B2F3 C15F

On Wed, Sep 30, 2015 at 8:20 AM, Philippe Blouin <philippe.blouin@inlibro.com> wrote:

Because the code would be extremely specific to a specific functionality, whereas the code in the libraries is there to be reused. And let not go overboard with OO.

A script is a script is a script. Make it readable, add functions to make it cleaner, make your functions readable.

And make some rule for forbidding direct DB accesses, so that future DB changes do not require to change many scripts. But please, do not forbid functions in scripts.

I tend to agree with Philipee here. It is a bit of an overkill to blanket require a single-script function to be moved over to a library. Perhaps a better approach would be to add some rules which set for the case where functions in scripts should be moved into the appropriate library versus when the definition of functions is acceptable in the script itself. Kind regards, Chris

On Wed, Sep 30, 2015 at 8:08 PM, David Cook <dcook@prosentient.com.au> wrote:

Sorry, Chris, but I'm not sure I understand what you're saying here.

Let me see if I can clarify it a bit.

I suppose I don't really think about it in terms of libraries but rather in terms of classes. In theory, a script should be able to be broken down into concepts which fit into classes.

In my understanding class implies a related (however loosely) set of data and methods. So to put the suggestion under discussion into practice one would need to essentially create a "module" for every script which contained any "methods" unique to that script or create a sort of catch-all module into which one would place methods which are generally unrelated to each other, but do not properly belong to any other module. Such a module would eventually become rather large and unmanageable I think. Take, for example, this script which produces the pdf file during a label export. http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=labels/label-creat... In it are two subs which might have been made methods of the C4::Labels::Label class, but they are not properly members of that class as they do not operate on a label object, but on the pdf object. Perhaps they could have gone into C4::Creators::PDF class, but that is really just a wrapper to make PDF::Reuse behave OO-like and those subs are totally unrelated to PDF::Reuse. It might go into C4::Creators::Lib as that is not properly a class, but a catch-all for functions to permit them to be called OO-style, but all of the functions in Lib are used by more than one script where the above mentioned are used only once. The script which produces the pdf file is not hard to debug/test as the vast majority of it is OO. The very little bit which is not is still rather straightforward. (BTW, are we talking about writing tests for every single script as well as the various modules? Maybe I'm not seeing something... which is a pretty common thing for me. :-)

I think sometimes we end up using subroutines in scripts because we haven't scrutinized the actual "function" enough. For instance, take a look at tools/letter.pl.

Another scary one is opac/oai.pl, although that's actually created packages in the pl script which I think is actually worse in a way.

I certainly agree with your observations in cases such as these. There are undoubtedly many things in Koha which could and should be refactored into OO form, and subs/functions are often used as an easy out by authors. The labels (and friends) code was my feeble attempt to do that in that section of the Koha world.

Maybe it's not feasible to always have subroutines defined in Perl Modules. However, I rather there be a blanket ban on them, and allow exceptions to that rule on the discretion of the QA team and the RM.

As Tomas has said, I think there's been an existing practice for some at least a few releases now to fail patches which have complex subroutines in Perl Files. This would simply be codifying an existing informal rule.

If it is already the practice of the QA team to examine each instance and pass/fail on the merits of the particular form based on the particular application, then it appears to me that there is no need for a ban. What seems more reasonable is a formal statement of what has been the practice: a case by case determination which requires the author to justify cases that the QA team fails. A ban seems to imply a complete unwillingness to consider the possibility of the potential need of a sub/function. Although I'm sure that is not at all the motive. Ultimately, I'll yield to whatever everyone thinks is best. This is just my humble (and probably uninformed) opinion. Kind regards, Chris

I agree with you all. The Release Team focuses on testability, and we'll keep doing that unless some radical change happens with the next release team election. So, as I said, this is already being encouraged (keeping things testable+tested) and no written rule is needed for us to work like we do now. Maybe it would make it explicit, but I've learnt that this community prefers discussions about ways to achieve a goal (or better, about the patches themselves), rather than hard rules on how to do things that can be done in different ways. That said, the patches for bug 13799 [1] (which is in really good shape now) show that even controller scripts can be tested if they are written with the right tools and testability in mind. It will be patch writter's job to convince the release team (as usual!) that there is a real need to write helper functions inside the controller/cli scripts. Kind regards [1] http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=13799 2015-10-01 0:14 GMT-03:00 David Cook <dcook@prosentient.com.au>:

Hi Chris:

Thanks for taking the time to explain your thoughts.

In the case of http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=labels/label-creat..., I probably would've structured the code differently. If you look at _print_text(), it looks like it should be a method of $pdf. Scoping $pdf with "our" seems suboptimal here although I imagine it was needed for it work at all with Plack? I'd have to look at the code more to understand "_calc_next_label_pos()", but it looks similar.

For me, we have a number of concepts here. The most obvious one is PDF. We're creating and outputting a PDF. So that's a conceptual class. But it's not any old PDF. It's a PDF of labels. So maybe we have something like PDF::Labels as another conceptual class... or maybe Labels::PDF as we might have other ways of building and outputting labels. So we subclass PDF and add methods for building a label sheet. Maybe we have other classes specific to individual labels and we add those to the label sheet object. I don't know. This is just me thinking outloud. Each class would resemble a "thing" and have properties and methods suitable to the what that "thing" does or how its structured. In this case, we're adding labels to the PDF, so the method for adding label text to the PDF would be in the PDF class. The code for the particulars might be in a PDF::Label or Label::PDF object which the PDF class knows how to handle.

Anyway, that's probably the last of my comments on the matter. I think the RM and QA team will continue to do what they do best, and that's make Koha the best it can be.

David Cook Systems Librarian Prosentient Systems 72/330 Wattle St, Ultimo, NSW 2007

...

-----Original Message----- From: Christopher Nighswonger [mailto:chris.nighswonger@gmail.com] Sent: Thursday, 1 October 2015 12:29 PM To: David Cook <dcook@prosentient.com.au> Cc: Philippe Blouin <philippe.blouin@inlibro.com>; Tomas Cohen Arazi <tomascohen@gmail.com>; Koha Devel <koha-devel@lists.koha- community.org> Subject: Re: [Koha-devel] Add rule for no subroutines in PL scripts

On Wed, Sep 30, 2015 at 8:08 PM, David Cook <dcook@prosentient.com.au> wrote:

...

Sorry, Chris, but I'm not sure I understand what you're saying here.

Let me see if I can clarify it a bit.

...

I suppose I don't really think about it in terms of libraries but rather in terms of classes. In theory, a script should be able to be broken down into concepts which fit into classes.

In my understanding class implies a related (however loosely) set of data and methods. So to put the suggestion under discussion into practice one would need to essentially create a "module" for every script which contained any "methods" unique to that script or create a sort of catch-all module into which one would place methods which are generally unrelated to each other, but do not properly belong to any other module. Such a module would eventually become rather large and unmanageable I think.

Take, for example, this script which produces the pdf file during a label export.

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=labels/label- create-pdf.pl;h=e69ba41e55634d5b15ad7a993edbd203b649424d;hb=HEAD

In it are two subs which might have been made methods of the C4::Labels::Label class, but they are not properly members of that class as they do not operate on a label object, but on the pdf object. Perhaps they could have gone into C4::Creators::PDF class, but that is really just a wrapper to make PDF::Reuse behave OO-like and those subs are totally unrelated to PDF::Reuse. It might go into C4::Creators::Lib as that is not properly a class, but a catch-all for functions to permit them to be called OO-style, but all of the functions in Lib are used by more than one script where the above mentioned are used only once.

The script which produces the pdf file is not hard to debug/test as the vast majority of it is OO. The very little bit which is not is still rather straightforward. (BTW, are we talking about writing tests for every single script as well as the various modules? Maybe I'm not seeing something... which is a pretty common thing for me. :-)

...

I think sometimes we end up using subroutines in scripts because we

haven't scrutinized the actual "function" enough. For instance, take a look at tools/letter.pl.

...

Another scary one is opac/oai.pl, although that's actually created

packages in the pl script which I think is actually worse in a way.

...

I certainly agree with your observations in cases such as these. There are undoubtedly many things in Koha which could and should be refactored into OO form, and subs/functions are often used as an easy out by authors. The labels (and friends) code was my feeble attempt to do that in that section of the Koha world.

...

Maybe it's not feasible to always have subroutines defined in Perl Modules. However, I rather there be a blanket ban on them, and allow exceptions to that rule on the discretion of the QA team and the RM.

As Tomas has said, I think there's been an existing practice for some at least a few releases now to fail patches which have complex subroutines in Perl Files. This would simply be codifying an existing informal rule.

If it is already the practice of the QA team to examine each instance and pass/fail on the merits of the particular form based on the particular application, then it appears to me that there is no need for a ban. What seems more reasonable is a formal statement of what has been the practice: a case by case determination which requires the author to justify cases that the QA team fails. A ban seems to imply a complete unwillingness to consider the possibility of the potential need of a sub/function. Although I'm sure that is not at all the motive.

Ultimately, I'll yield to whatever everyone thinks is best. This is just my humble (and probably uninformed) opinion.

Kind regards, Chris

-- Tomás Cohen Arazi Theke Solutions (http://theke.io) ✆ +54 9351 3513384 GPG: B76C 6E7C 2D80 551A C765 E225 0A27 2EA1 B2F3 C15F

Agreed. Thanks for creating that bug, Frédéric! David Cook Systems Librarian Prosentient Systems 72/330 Wattle St, Ultimo, NSW 2007

-----Original Message----- From: frederic.demians@gmail.com [mailto:frederic.demians@gmail.com] On Behalf Of Frédéric Demians Sent: Friday, 2 October 2015 4:59 PM To: David Cook <dcook@prosentient.com.au> Cc: Christopher Nighswonger <chris.nighswonger@gmail.com>; Philippe Blouin <philippe.blouin@inlibro.com>; Koha Devel <koha-devel@lists.koha- community.org> Subject: Re: [Koha-devel] Add rule for no subroutines in PL scripts

...

Another scary one is opac/oai.pl, although that's actually created packages in the pl script which I think is actually worse in a way.

Even if I am on Philippe Blouin side on this discusion (overengineering has its drawbacks), concerning Koha OAI server, it has been done in one unique .pl file because there wasn't any object model or rules in the Koha project when it has been coded. But it's OO (with something like Moose, it would be even cleaner...) It's just a matter of modularization: putting each class in a separate file in a Koha::OAI namespace.

I've created a bug for that:

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14939