[Koha-devel] Bug 18539 - Forbid Koha::Objects->find calls in list context
David Cook
dcook at prosentient.com.au
Fri Dec 15 00:24:45 CET 2017
Fixing the calls would make the behaviour of the function more explicit and less prone to human error.
But... I think fixing the function itself is fair enough. If we look at DBIx::Class::ResultSet, it looks like it returns a scalar or undef: http://search.cpan.org/dist/DBIx-Class/lib/DBIx/Class/ResultSet.pm#find. Makes sense if Koha::Objects does the same thing.
David Cook
Systems Librarian
Prosentient Systems
72/330 Wattle St
Ultimo, NSW 2007
Australia
Office: 02 9212 0899
Direct: 02 8005 0595
-----Original Message-----
From: Julian Maurice [mailto:julian.maurice at biblibre.com]
Sent: Thursday, 14 December 2017 7:26 PM
To: David Cook <dcook at prosentient.com.au>; 'Marcel de Rooy' <M.de.Rooy at rijksmuseum.nl>
Cc: koha-devel at lists.koha-community.org
Subject: Re: [Koha-devel] Bug 18539 - Forbid Koha::Objects->find calls in list context
Of course we could, but why fix the calls when we can fix the subroutine itself ?
I filed a bug report :
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19809
Le 14/12/2017 à 00:51, David Cook a écrit :
> Alternatively, you could write:
>
> my $foo = Koha::Patrons->find("foo");
> my $hashref = { a => $foo, b => "bar" };
>
> or
> my $foo = Koha::Patrons->find("foo");
> my $hashref = { b => "bar" };
> $hashref->{a} = $foo if $foo;
>
> Personally, I'm not a fan of putting function calls in data structures.
>
> David Cook
> Systems Librarian
> Prosentient Systems
> 72/330 Wattle St
> Ultimo, NSW 2007
> Australia
>
> Office: 02 9212 0899
> Direct: 02 8005 0595
>
> -----Original Message-----
> From: koha-devel-bounces at lists.koha-community.org
> [mailto:koha-devel-bounces at lists.koha-community.org] On Behalf Of
> Julian Maurice
> Sent: Thursday, 14 December 2017 1:57 AM
> To: Marcel de Rooy <M.de.Rooy at rijksmuseum.nl>
> Cc: koha-devel at lists.koha-community.org
> Subject: Re: [Koha-devel] Bug 18539 - Forbid Koha::Objects->find calls
> in list context
>
> Probably, but that's not my point.
>
> I understand why the previous code needed to be fixed, which is to
> avoid that
>
> { a => Koha::Patrons->find("foo"), b => "bar" }
>
> would result in { a => "b", "bar" => undef }
>
> My question is: why the solution to this problem was to forbid calls
> in list context while we could simply return an explicit undef?
> It forces us to write 'scalar' in front of a call that should always
> return a single value, and in my opinion it's a "code smell"
>
> Le 13/12/2017 à 15:29, Marcel de Rooy a écrit :
>> i forgot to add scalar in the second..
>>
>> probably you should
>>
>> if( $a = xxx->find )
>>
>> push @b, $a
>>
>>
>> ---------------------------------------------------------------------
>> -
>> --
>> *Van:* Julian Maurice <julian.maurice at biblibre.com>
>> *Verzonden:* woensdag 13 december 2017 15:20:34
>> *Aan:* Marcel de Rooy
>> *Onderwerp:* Re: [Koha-devel] Bug 18539 - Forbid Koha::Objects->find
>> calls in list context
>>
>> This particular line will work (only the first, in the second, find
>> is still called in list context), but @a would contain one element
>> (undef) and the example code given by Jonathan will also fail,
>> whether we call find in list context or not.
>>
>> Le 13/12/2017 à 15:12, Marcel de Rooy a écrit :
>>> Are you sure ?
>>>
>>> push @a, scalar Koha..->find
>>>
>>> or @a = ( ...->find )
>>>
>>>
>>> should work imo
>>>
>>> --------------------------------------------------------------------
>>> -
>>> ---
>>> *Van:* koha-devel-bounces at lists.koha-community.org
>>> <koha-devel-bounces at lists.koha-community.org> namens Julian Maurice
>>> <julian.maurice at biblibre.com>
>>> *Verzonden:* woensdag 13 december 2017 15:07:11
>>> *Aan:* Jonathan Druart
>>> *CC:* koha-devel at lists.koha-community.org
>>> *Onderwerp:* Re: [Koha-devel] Bug 18539 - Forbid Koha::Objects->find
>>> calls in list context
>>>
>>> Ok, but the proposed fix doesn't fix this code:
>>>
>>> @a = scalar Koha::Patrons->find('foo'); if (@a) { # still equals to
>>> 1
>>> say $a[0]->borrowernumber; # still BOOM }
>>>
>>> And yes, that was the link ;)
>>>
>>> Le 13/12/2017 à 14:58, Jonathan Druart a écrit :
>>>> Hi Julian,
>>>>
>>>>
>>>>> @a = Koha::Patrons->find('foo'); # would result in @a = (undef)
>>>>
>>>> And that leads to issues:
>>>> if ( @a ) { # = 1
>>>> say $a[0]->borrowernumber; # BOOM }
>>>>
>>>> See also https://perlmaven.com/how-to-return-undef-from-a-function
>>>> (was it the link you were talking about?)
>>>>
>>>> Cheers,
>>>> Jonathan
>>>>
>>>> On Wed, 13 Dec 2017 at 10:34 Julian Maurice
>>>> <julian.maurice at biblibre.com <mailto:julian.maurice at biblibre.com>>
> wrote:
>>>>
>>>> Hi developers,
>>>>
>>>> I stumbled upon a line of code recently and I can't figure out
>>>> why it
>>>> has be done this way. I hope you can help me :)
>>>>
>>>> The line in question is in Koha::Objects::find:
>>>>
>>>> croak 'Cannot use "->find" in list context' if wantarray;
>>>>
>>>> I read the two bugs (18539 and 18179) and the link given by
>>>> Jonathan but
>>>> I still don't understand why the call in list context has been
>>>> forbidden. Why not simply return undef (an explicit undef)
>>>> when no
>>>> records have be found ? It would work as expected in scalar
>>>> and list
>>>> contexts.
>>>>
>>>> Here is a possible rewrite of 'find' to better explain what I mean:
>>>>
>>>> sub find {
>>>> my ( $self, @pars ) = @_;
>>>>
>>>> my $object = undef;
>>>>
>>>> @pars = grep { defined } @pars;
>>>> if (@pars) {
>>>> my $result = $self->_resultset()->find(@pars);
>>>> if ($result) {
>>>> $object =
>>>> $self->object_class()->_new_from_dbic($result);
>>>> }
>>>> }
>>>>
>>>> return $object;
>>>> }
>>>>
>>>> @a = Koha::Patrons->find('foo'); # would result in @a =
>>>> (undef)
>>>> {a => K::P->find('foo'), b => 'bar'}; # would result in {a =>
>>>> undef, b
>>>> => 'bar'}
>>>>
>>>> Please tell me what you think.
>>>>
>>>> --
>>>> Julian Maurice <julian.maurice at biblibre.com
>>>> <mailto:julian.maurice at biblibre.com>>
>>>> BibLibre
>>>> _______________________________________________
>>>> Koha-devel mailing list
>>>> Koha-devel at lists.koha-community.org
>>>> <mailto:Koha-devel at lists.koha-community.org>
>>>>
>>>> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
>>>> website : http://www.koha-community.org/
>>>> git : http://git.koha-community.org/
>>>> bugs : http://bugs.koha-community.org/
>>>>
>>>
>>> --
>>> Julian Maurice <julian.maurice at biblibre.com> BibLibre
>>> _______________________________________________
>>> Koha-devel mailing list
>>> Koha-devel at lists.koha-community.org
>>> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
>>> website : http://www.koha-community.org/ git :
>>> http://git.koha-community.org/ bugs :
>>> http://bugs.koha-community.org/
>>
>> --
>> Julian Maurice <julian.maurice at biblibre.com> BibLibre
>
> --
> Julian Maurice <julian.maurice at biblibre.com> BibLibre
> _______________________________________________
> Koha-devel mailing list
> Koha-devel at lists.koha-community.org
> http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-devel
> website : http://www.koha-community.org/ git :
> http://git.koha-community.org/ bugs : http://bugs.koha-community.org/
>
>
--
Julian Maurice <julian.maurice at biblibre.com> BibLibre
More information about the Koha-devel
mailing list