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@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@lists.koha-community.org <koha-devel-bounces@lists.koha-community.org> namens Julian Maurice <julian.maurice@biblibre.com> *Verzonden:* woensdag 13 december 2017 15:07:11 *Aan:* Jonathan Druart *CC:* koha-devel@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@biblibre.com <mailto:julian.maurice@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@biblibre.com <mailto:julian.maurice@biblibre.com>> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org <mailto:Koha-devel@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@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
-- Julian Maurice <julian.maurice@biblibre.com> BibLibre
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
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@lists.koha-community.org [mailto:koha-devel-bounces@lists.koha-community.org] On Behalf Of Julian Maurice Sent: Thursday, 14 December 2017 1:57 AM To: Marcel de Rooy <M.de.Rooy@rijksmuseum.nl> Cc: koha-devel@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@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@lists.koha-community.org <koha-devel-bounces@lists.koha-community.org> namens Julian Maurice <julian.maurice@biblibre.com> *Verzonden:* woensdag 13 december 2017 15:07:11 *Aan:* Jonathan Druart *CC:* koha-devel@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@biblibre.com <mailto:julian.maurice@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@biblibre.com <mailto:julian.maurice@biblibre.com>> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org <mailto:Koha-devel@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@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
-- Julian Maurice <julian.maurice@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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/
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@lists.koha-community.org [mailto:koha-devel-bounces@lists.koha-community.org] On Behalf Of Julian Maurice Sent: Thursday, 14 December 2017 1:57 AM To: Marcel de Rooy <M.de.Rooy@rijksmuseum.nl> Cc: koha-devel@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@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@lists.koha-community.org <koha-devel-bounces@lists.koha-community.org> namens Julian Maurice <julian.maurice@biblibre.com> *Verzonden:* woensdag 13 december 2017 15:07:11 *Aan:* Jonathan Druart *CC:* koha-devel@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@biblibre.com <mailto:julian.maurice@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@biblibre.com <mailto:julian.maurice@biblibre.com>> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org <mailto:Koha-devel@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@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
-- Julian Maurice <julian.maurice@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
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@biblibre.com] Sent: Thursday, 14 December 2017 7:26 PM To: David Cook <dcook@prosentient.com.au>; 'Marcel de Rooy' <M.de.Rooy@rijksmuseum.nl> Cc: koha-devel@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@lists.koha-community.org [mailto:koha-devel-bounces@lists.koha-community.org] On Behalf Of Julian Maurice Sent: Thursday, 14 December 2017 1:57 AM To: Marcel de Rooy <M.de.Rooy@rijksmuseum.nl> Cc: koha-devel@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@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@lists.koha-community.org <koha-devel-bounces@lists.koha-community.org> namens Julian Maurice <julian.maurice@biblibre.com> *Verzonden:* woensdag 13 december 2017 15:07:11 *Aan:* Jonathan Druart *CC:* koha-devel@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@biblibre.com <mailto:julian.maurice@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@biblibre.com <mailto:julian.maurice@biblibre.com>> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@lists.koha-community.org <mailto:Koha-devel@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@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
-- Julian Maurice <julian.maurice@biblibre.com> BibLibre _______________________________________________ Koha-devel mailing list Koha-devel@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@biblibre.com> BibLibre
participants (3)
-
David Cook -
Francesco Rivetti -
Julian Maurice