Bug 18539 - Forbid Koha::Objects->find calls in list context

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug 18539 - Forbid Koha::Objects->find calls in list context

Julian Maurice
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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Marcel de Rooy

Find is supposed for retrieving one result not multiple ones. Use search instead.

Using find in a list context is not correct.


Van: [hidden email] <[hidden email]> namens Julian Maurice <[hidden email]>
Verzonden: woensdag 13 december 2017 14:34:07
Aan: [hidden email]
Onderwerp: [Koha-devel] Bug 18539 - Forbid Koha::Objects->find calls in list context
 
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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/

_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Julian Maurice
I know that find is supposed to retrieve only one result. That's why we
shouldn't have to care about the context, it's never ambiguous.
I should be allowed to write

    push @patrons, Koha::Patrons->find($borrowernumber);

without specifying that I only want one record (using 'scalar')

Le 13/12/2017 à 14:50, Marcel de Rooy a écrit :

> Find is supposed for retrieving one result not multiple ones. Use search
> instead.
>
> Using find in a list context is not correct.
>
> ------------------------------------------------------------------------
> *Van:* [hidden email]
> <[hidden email]> namens Julian Maurice
> <[hidden email]>
> *Verzonden:* woensdag 13 december 2017 14:34:07
> *Aan:* [hidden email]
> *Onderwerp:* [Koha-devel] Bug 18539 - Forbid Koha::Objects->find calls
> in list context
>  
> 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 <[hidden email]>
> BibLibre
> _______________________________________________
> Koha-devel mailing list
> [hidden email]
> 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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Jonathan Druart
In reply to this post by Julian Maurice
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 <[hidden email]> 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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/

_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Julian Maurice
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 <[hidden email]
> <mailto:[hidden email]>> 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 <[hidden email]
>     <mailto:[hidden email]>>
>     BibLibre
>     _______________________________________________
>     Koha-devel mailing list
>     [hidden email]
>     <mailto:[hidden email]>
>     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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Julian Maurice
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 <[hidden email]>
> *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:* [hidden email]
>> <[hidden email]> namens Julian Maurice
>> <[hidden email]>
>> *Verzonden:* woensdag 13 december 2017 15:07:11
>> *Aan:* Jonathan Druart
>> *CC:* [hidden email]
>> *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 <[hidden email]
>>> <mailto:[hidden email]>> 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 <[hidden email]
>>>     <mailto:[hidden email]>>
>>>     BibLibre
>>>     _______________________________________________
>>>     Koha-devel mailing list
>>>     [hidden email]
>>>     <mailto:[hidden email]>
>>>     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 <[hidden email]>
>> BibLibre
>> _______________________________________________
>> Koha-devel mailing list
>> [hidden email]
>> 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 <[hidden email]>
> BibLibre

--
Julian Maurice <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Francesco Rivetti
On 13. des. 2017 15:56, Julian Maurice wrote:
> 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"

I completely agree with this.

Francesco
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

David Cook
In reply to this post by Julian Maurice
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: [hidden email]
[mailto:[hidden email]] On Behalf Of Julian
Maurice
Sent: Thursday, 14 December 2017 1:57 AM
To: Marcel de Rooy <[hidden email]>
Cc: [hidden email]
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 <[hidden email]>
> *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:* [hidden email]
>> <[hidden email]> namens Julian Maurice
>> <[hidden email]>
>> *Verzonden:* woensdag 13 december 2017 15:07:11
>> *Aan:* Jonathan Druart
>> *CC:* [hidden email]
>> *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
>>> <[hidden email] <mailto:[hidden email]>>
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 <[hidden email]
>>>     <mailto:[hidden email]>>
>>>     BibLibre
>>>     _______________________________________________
>>>     Koha-devel mailing list
>>>     [hidden email]
>>>     <mailto:[hidden email]>
>>>    
>>>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 <[hidden email]> BibLibre
>> _______________________________________________
>> Koha-devel mailing list
>> [hidden email]
>> 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 <[hidden email]> BibLibre

--
Julian Maurice <[hidden email]> BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/


_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

Julian Maurice
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: [hidden email]
> [mailto:[hidden email]] On Behalf Of Julian
> Maurice
> Sent: Thursday, 14 December 2017 1:57 AM
> To: Marcel de Rooy <[hidden email]>
> Cc: [hidden email]
> 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 <[hidden email]>
>> *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:* [hidden email]
>>> <[hidden email]> namens Julian Maurice
>>> <[hidden email]>
>>> *Verzonden:* woensdag 13 december 2017 15:07:11
>>> *Aan:* Jonathan Druart
>>> *CC:* [hidden email]
>>> *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
>>>> <[hidden email] <mailto:[hidden email]>>
> 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 <[hidden email]
>>>>      <mailto:[hidden email]>>
>>>>      BibLibre
>>>>      _______________________________________________
>>>>      Koha-devel mailing list
>>>>      [hidden email]
>>>>      <mailto:[hidden email]>
>>>>     
>>>> 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 <[hidden email]> BibLibre
>>> _______________________________________________
>>> Koha-devel mailing list
>>> [hidden email]
>>> 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 <[hidden email]> BibLibre
>
> --
> Julian Maurice <[hidden email]> BibLibre
> _______________________________________________
> Koha-devel mailing list
> [hidden email]
> 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 <[hidden email]>
BibLibre
_______________________________________________
Koha-devel mailing list
[hidden email]
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/
Reply | Threaded
Open this post in threaded view
|

Re: Bug 18539 - Forbid Koha::Objects->find calls in list context

David Cook
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:[hidden email]]
Sent: Thursday, 14 December 2017 7:26 PM
To: David Cook <[hidden email]>; 'Marcel de Rooy' <[hidden email]>
Cc: [hidden email]
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: [hidden email]
> [mailto:[hidden email]] On Behalf Of
> Julian Maurice
> Sent: Thursday, 14 December 2017 1:57 AM
> To: Marcel de Rooy <[hidden email]>
> Cc: [hidden email]
> 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 <[hidden email]>
>> *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:* [hidden email]
>>> <[hidden email]> namens Julian Maurice
>>> <[hidden email]>
>>> *Verzonden:* woensdag 13 december 2017 15:07:11
>>> *Aan:* Jonathan Druart
>>> *CC:* [hidden email]
>>> *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
>>>> <[hidden email] <mailto:[hidden email]>>
> 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 <[hidden email]
>>>>      <mailto:[hidden email]>>
>>>>      BibLibre
>>>>      _______________________________________________
>>>>      Koha-devel mailing list
>>>>      [hidden email]
>>>>      <mailto:[hidden email]>
>>>>      
>>>> 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 <[hidden email]> BibLibre
>>> _______________________________________________
>>> Koha-devel mailing list
>>> [hidden email]
>>> 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 <[hidden email]> BibLibre
>
> --
> Julian Maurice <[hidden email]> BibLibre
> _______________________________________________
> Koha-devel mailing list
> [hidden email]
> 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 <[hidden email]> BibLibre


_______________________________________________
Koha-devel mailing list
[hidden email]
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/