[Koha-devel] Add rule for no subroutines in PL scripts

[Tomas Cohen Arazi]

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 at 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-create-pdf.pl;h=e69ba41e55634d5b15ad7a993edbd203b649424d;hb=HEAD,
> 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 at gmail.com]
> > Sent: Thursday, 1 October 2015 12:29 PM
> > To: David Cook <dcook at prosentient.com.au>
> > Cc: Philippe Blouin <philippe.blouin at inlibro.com>; Tomas Cohen Arazi
> > <tomascohen at gmail.com>; Koha Devel <koha-devel at 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 at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.koha-community.org/pipermail/koha-devel/attachments/20151001/d50567b4/attachment.html>