See bug 23006 and bug 22941.
See also bug 17526 to deal with OPAC lists.
Created attachment 90820 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo!
Created attachment 90821 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo!
Created attachment 90823 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo!
Comment on attachment 90823 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} Review of attachment 90823 [details] [review]: ----------------------------------------------------------------- I only thing that really stood out was the C4/Search.pm "Why is the \Q and \E missing?"? Explain that, I may sign this off. Everything else is a result of me seeing tcohen wanting to improve readability of code. "unless grep" is better read as "if none". ::: C4/ImportBatch.pm @@ +1114,4 @@ > my $dbh = C4::Context->dbh; > > my $order_by = $parameters->{order_by} || 'import_record_id'; > + ( $order_by ) = grep( { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id'; Wouldn't 'any' be semantically easier to read? It also has the added benefit of removing the whole need for () around $order_by, because $order_by is a string. $order_by = any { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? $order_by : 'import_record_id'; Actually, the whole point of the code is even clearer with 'none': $order_by = none { $_ eq $order_by } qw( import_record_id title status overlay_status ) ) ? 'import_record_id' : $order_by; ::: C4/Installer/PerlModules.pm @@ +76,4 @@ > no warnings > ; # perl throws warns for invalid $VERSION numbers which some modules use > my ( $self, $module ) = @_; > + return -1 unless grep { $_ eq $module } keys(%$PERL_DEPS); any is better, since it is boolean. ::: C4/Items.pm @@ +2101,4 @@ > push @columns, Koha::Database->new()->schema()->resultset('Biblioitem')->result_source->columns; > my @operators = qw(= != > < >= <= like); > my $field = $filter->{field}; > + if ( (0 < grep { $_ eq $field } @columns) or (substr($field, 0, 5) eq 'marc:') ) { any is better, since it checking if the field is in the list of columns. @@ +2105,4 @@ > my $op = $filter->{operator}; > my $query = $filter->{query}; > > + if (!$op or (0 == grep { $_ eq $op } @operators)) { none would be better than a '0 ==' check. @@ +2601,5 @@ > |; > for my $condition ( @$conditions ) { > if ( > + grep { $_ eq $condition->{field} } @item_columns > + or grep { $_ eq $condition->{field} } @biblioitem_columns any's would be nicer. ::: C4/OAI/Sets.pm @@ +500,4 @@ > my $value = $mapping->{'marcvalue'}; > my @subfield_values = $record->subfield($field, $subfield); > if ($operator eq 'notequal') { > + if(0 == grep { $_ eq $value } @subfield_values) { none. @@ +505,5 @@ > last; > } > } > else { > + if(0 < grep { $_ eq $value } @subfield_values) { any. ::: C4/Overdues.pm @@ +766,4 @@ > # Put 'print' in first if exists > # It avoid to sent a print notice with an email or sms template is no email or sms is defined > @mtts = uniq( 'print', @mtts ) > + if grep { $_ eq 'print' } @mtts; any. ::: C4/Search.pm @@ +731,4 @@ > > my $data = $field->as_string( $subfield_letters, $facet->{ sep } ); > > + unless ( grep { $_ eq $data } @used_datas ) { Why is the \Q and \E missing? @@ +1490,4 @@ > # this happens when selecting a subject on the opac-detail page > @limits = grep {!/^$/} @limits; > my $original_q = $q; # without available part > + unless ( grep { $_ eq 'available' } @limits ) { unless is harder to read construction, if none would be clearer, I think. @@ +1494,5 @@ > $q =~ s| and \( \( allrecords,AlwaysMatches:'' not onloan,AlwaysMatches:''\) and \(lost,st-numeric=0\) \)||; > $original_q = $q; > } > if ( @limits ) { > + if ( grep { $_ eq 'available' } @limits ) { any. ::: C4/Serials.pm @@ +1579,4 @@ > ### Would use substr and index But be careful to previous presence of () > $recievedlist .= "; $serialseq" unless ( index( $recievedlist, $serialseq ) > 0 ); > } > + if ( grep { $_ eq $status } (MISSING_STATUSES) ) { any. ::: C4/Stats.pm @@ +108,4 @@ > } > my @invalid_params = (); > for my $myparam (keys %$params ) { > + push @invalid_params, $myparam unless grep { $_ eq $myparam } @allowed_keys; if none would be easier to read. ::: C4/Tags.pm @@ +192,4 @@ > carp "Empty argument key to get_tag_rows: ignoring!"; > next; > } > + unless (1 == scalar grep { $_ eq $key } @ok_fields) { if none would be easier to read. @@ +233,4 @@ > carp "Empty argument key to get_tags: ignoring!"; > next; > } > + unless (1 == scalar grep { $_ eq $key } @ok_fields) { if none would be easier to read. @@ +302,4 @@ > carp "Empty argument key to get_approval_rows: ignoring!"; > next; > } > + unless (1 == scalar grep { $_ eq $key } @ok_fields) { if none would be easier to read. ::: C4/Utils/DataTables/Members.pm @@ +38,4 @@ > # Do that after iTotalQuery! > if ( defined $branchcode and $branchcode ) { > @restricted_branchcodes = @restricted_branchcodes > + ? grep { $_ eq $branchcode } @restricted_branchcodes @restricted_branchcodes = ((scalar @restricted_branchcodes > 0) && (none { $_ eq $branchcode } @restricted_branchcodes)) ? (undef) : ($branchcode); Nested ternary's are harder to read. ::: Koha/Object.pm @@ +236,4 @@ > my @columns = @{$self->_columns()}; > > foreach my $p ( keys %$properties ) { > + unless ( grep { $_ eq $p } @columns ) { if none. @@ +442,4 @@ > > my @columns = @{$self->_columns()}; > # Using direct setter/getter like $item->barcode() or $item->barcode($barcode); > + if ( grep { $_ eq $method } @columns ) { if any. @@ +457,4 @@ > Koha::Exceptions::Object::MethodNotCoveredByTests->throw( > error => sprintf("The method %s->%s is not covered by tests!", ref($self), $method), > show_trace => 1 > + ) unless grep { $_ eq $method } @known_methods; if none. ::: Koha/Objects.pm @@ +384,4 @@ > $method =~ s/.*:://; > > > + unless ( grep { $_ eq $method } @known_methods ) { if none. ::: acqui/duplicate_orders.pl @@ +86,4 @@ > if ( $op eq 'select' ) { > @result_order_loop = map { > my $order = $_; > + ( grep {$_ eq $order->{ordernumber}} @ordernumbers ) ? () : $order Too ugly to suggest something. @@ +132,4 @@ > for my $field ( > qw(currency budget_id order_internalnote order_vendornote sort1 sort2 )) > { > + next if grep { $_ eq $field } @fields_to_copy; any. ::: admin/preferences.pl @@ +117,4 @@ > { > text => $options{multiple}->{$option_value}, > value => $option_value, > + selected => (grep { $_ eq $option_value } @values) ? 1 : 0, any without the ternary? ::: admin/searchengine/elasticsearch/mappings.pl @@ +124,4 @@ > my $mapping_suggestible = $mapping_suggestible[$i]; > my $mapping_sort = $mapping_sort[$i]; > $mapping_sort = undef if $mapping_sort eq 'undef'; > + $mapping_facet = ( grep { $_ eq $search_field_name } @facetable_field_names ) ? $mapping_facet : 0; $mapping_facet = 0 if none { $_ eq $search_field_name } @facetable_field_names; @@ +200,4 @@ > sort => $s->get_column('sort') // 'undef', # To avoid warnings "Use of uninitialized value in lc" > suggestible => $s->get_column('suggestible'), > facet => $s->get_column('facet'), > + is_facetable => ( grep { $_ eq $name } @facetable_field_names ) ? 1 : 0, any without the ternary? ::: catalogue/ISBDdetail.pl @@ +152,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_deletedorders){ if none. @@ +157,5 @@ > } > } > else { > push @orders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_orders){ if none, and fix the brace indentation below. ::: catalogue/MARCdetail.pl @@ +328,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_deletedorders){ if none @@ +333,5 @@ > } > } > else { > push @orders_using_biblio, $myorder; > + unless (grep { $_ eq $basket } @baskets_orders){ if none with brace indentation fix below. ::: catalogue/detail.pl @@ +522,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_deletedorders){ if none @@ +527,5 @@ > } > } > else { > push @orders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_orders){ if none with brace indentation below. ::: catalogue/imageviewer.pl @@ +89,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_deletedorders){ if none @@ +99,1 @@ > push @baskets_orders,$myorder->{'basketno'}; if none with brace indentation below. ::: catalogue/itemsearch.pl @@ +63,4 @@ > push @f, $columns[$i]; > push @c, 'and'; > > + if ( grep { $_ eq $columns[$i] } qw( ccode homebranch holdingbranch location itype notforloan itemlost ) ) { any would be better. ::: catalogue/labeledMARCdetail.pl @@ +135,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep { $_ eq $basket } @baskets_deletedorders){ if none @@ +140,5 @@ > } > } > else { > push @orders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_orders){ if none with brace indentation fix below. ::: catalogue/moredetail.pl @@ +235,4 @@ > my $basket = $myorder->{'basketno'}; > if ((defined $myorder->{'datecancellationprinted'}) and ($myorder->{'datecancellationprinted'} ne '0000-00-00') ){ > push @deletedorders_using_biblio, $myorder; > + unless (grep{ $_ eq $basket } @baskets_deletedorders){ if none @@ +240,5 @@ > } > } > else { > push @orders_using_biblio, $myorder; > + unless (grep { $_ eq $basket } @baskets_orders){ if none with brace fix indentation below. ::: circ/circulation.pl @@ +126,4 @@ > $template_name = q|circ/circulation_batch_checkouts.tt|; > my @batch_category_codes = split '\|', C4::Context->preference('BatchCheckoutsValidCategories'); > my $categorycode = $patron->categorycode; > + if ( $categorycode && grep { $_ eq $categorycode } @batch_category_codes ) { any Though, I wonder if this $batch_allowed and $barcodes[] could be optimized a bit with the any logic. ::: clubs/templates-add-modify.pl @@ +92,4 @@ > ? Koha::Club::Template::Fields->find($field_id) > : Koha::Club::Template::Field->new(); > > + if ( grep { $_ eq $field_id } @field_delete ) { any @@ +126,4 @@ > ? Koha::Club::Template::EnrollmentFields->find($field_id) > : Koha::Club::Template::EnrollmentField->new(); > > + if ( grep { $_ eq $field_id } @field_delete ) { any ::: misc/cronjobs/gather_print_notices.pl @@ +91,4 @@ > @all_messages = map { > my $letter_code = $_->{letter_code}; > ( > + grep { $_ eq $letter_code } @letter_codes ugly code map code. no patently obvious suggestions. ::: misc/cronjobs/longoverdue.pl @@ +301,4 @@ > $skip_borrower_category = [ map { uc $_} @$skip_borrower_category ]; > my %category_to_process; > for my $cat ( @$borrower_category ) { > + unless ( grep { $_ eq $cat } @available_categories ) { if none @@ +311,4 @@ > } > if ( @$skip_borrower_category ) { > for my $cat ( @$skip_borrower_category ) { > + unless ( grep { $_ eq $cat } @available_categories ) { if none @@ +329,4 @@ > $skip_itemtype = [ map { uc $_} @$skip_itemtype ]; > my %itemtype_to_process; > for my $it ( @$itemtype ) { > + unless ( grep { $_ eq $it } @available_itemtypes ) { if none @@ +339,4 @@ > } > if ( @$skip_itemtype ) { > for my $it ( @$skip_itemtype ) { > + unless ( grep { $_ eq $it } @available_itemtypes ) { if none ::: misc/migration_tools/rebuild_zebra.pl @@ +146,4 @@ > } > > our @tables_allowed_for_select = ( 'biblioitems', 'items', 'biblio', 'biblio_metadata' ); > +unless ( grep { $_ eq $table } @tables_allowed_for_select ) { if none @@ +476,4 @@ > > sub select_all_biblios { > $table = 'biblioitems' > + unless grep { $_ eq $table } @tables_allowed_for_select; if none ::: misc/translator/LangInstaller.pm @@ +634,4 @@ > next if $entry =~ /^\./; > my $relentry = File::Spec->catfile($dir, $entry); > my $abspath = File::Spec->catfile($basedir, $relentry); > + if (-d $abspath and not grep { $_ eq $relentry } @blacklist) { none ::: opac/opac-detail.pl @@ +720,4 @@ > > if ( C4::Context->preference('OPACAcquisitionDetails') ) { > $itm->{on_order} = 1 > + if grep { $_ eq $itm->{itemnumber} } @itemnumbers_on_order; any. possible optimization? ::: opac/opac-memberentry.pl @@ +77,4 @@ > > my @libraries = Koha::Libraries->search; > if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) { > + @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep { $_ eq $branchcode } @libraries_to_display ? $b : () } @libraries; ugly code! no suggestions. ::: opac/opac-search-history.pl @@ +70,4 @@ > @searches = map { $_->{type} ne $type ? $_ : () } @searches; > } > if ( @id ) { > + @searches = map { my $search = $_; ( grep { $_ eq $search->{id} } @id ) ? () : $_ } @searches; so many ugly map{ grep {...}}'s. ::: opac/opac-search.pl @@ +431,4 @@ > @sort_by = $cgi->multi_param('sort_by'); > $sort_by[0] = $default_sort_by if !$sort_by[0] && defined($default_sort_by); > foreach my $sort (@sort_by) { > + if ( grep { $_ eq $sort } @allowed_sortby ) { any ::: opac/opac-shelves.pl @@ +114,3 @@ > if ( $shelf ) { > $op = $referer; > my $sortfield = $query->param('sortfield'); if $sortfield is undef, next line will explode. // 'title'; would be good. @@ +114,4 @@ > if ( $shelf ) { > $op = $referer; > my $sortfield = $query->param('sortfield'); > + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded ); if none. ::: reports/borrowers_stats.pl @@ +162,4 @@ > my $attribute_type = $1; > return unless (grep {$attribute_type eq $_->{code}} @attribute_types); > } else { > + return unless (grep { $_ eq $line } @valid_names); if none @@ +167,5 @@ > if ($column =~ /^patron_attr\.(.*)/) { > my $attribute_type = $1; > return unless (grep {$attribute_type eq $_->{code}} @attribute_types); > } else { > + return unless (grep { $_ eq $column } @valid_names); if none @@ +172,3 @@ > } > return if ($digits and $digits !~ /^\d+$/); > + return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0); none @@ +172,4 @@ > } > return if ($digits and $digits !~ /^\d+$/); > + return if ($status and (grep { $_ eq $status } qw(debarred gonenoaddress lost)) == 0); > + return if ($activity and (grep { $_ eq $activity } qw(active nonactive)) == 0); none ::: tools/export.pl @@ +75,4 @@ > if ( $filename ) { > my $mimetype = $query->uploadInfo($filename)->{'Content-Type'}; > my @valid_mimetypes = qw( application/octet-stream text/csv text/plain application/vnd.ms-excel ); > + unless ( grep { $_ eq $mimetype } @valid_mimetypes ) { if none ::: tools/letter.pl @@ +263,4 @@ > my $preview_is_available = 0; > > if ($code) { > + $preview_is_available = grep {$_ eq $code } qw( CHECKIN CHECKOUT HOLD_SLIP ); could optimize with an any. ::: tools/modborrowers.pl @@ +278,4 @@ > for my $field ( qw/surname firstname branchcode categorycode city state zipcode country sort1 sort2 dateenrolled dateexpiry borrowernotes opacnote/ ) { > my $value = $input->param($field); > $infos->{$field} = $value if $value; > + $infos->{$field} = "" if grep { $_ eq $field } @disabled; if (any...) { $infos->{$field} = q{}; } else { $infos->{$field} = $value if $value; } -- double assign shorter vertically, but two assigns are slower than one. Plus, any is better than grep. @@ +313,4 @@ > # If this borrower is not in the category of this attribute, we don't want to modify this attribute > ++$i and next if $attr_type->{category_code} and $attr_type->{category_code} ne $borrower_categorycode; > my $valuename = "attr" . $i . "_value"; > + if ( grep { $_ eq $valuename } @disabled ) { any ::: virtualshelves/shelves.pl @@ +102,4 @@ > if ( $shelf ) { > $op = $referer; > my $sortfield = $query->param('sortfield'); > + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded ); if none. also might want a "|| 'title'" added above. @@ +234,3 @@ > if ( $shelf ) { > if ( $shelf->can_be_viewed( $loggedinuser ) ) { > my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title'; # Passed in sorting overrides default sorting Actually, the following line is switch to title as default if the passed sortfield isn't one of the valid ones. Comment is wrong. @@ +234,4 @@ > if ( $shelf ) { > if ( $shelf->can_be_viewed( $loggedinuser ) ) { > my $sortfield = $query->param('sortfield') || $shelf->sortfield || 'title'; # Passed in sorting overrides default sorting > + $sortfield = 'title' unless grep { $_ eq $sortfield } qw( title author copyrightdate itemcallnumber dateadded ); if none.
(In reply to M. Tompsett from comment #5) > I only thing that really stood out was the C4/Search.pm > "Why is the \Q and \E missing?"? > > Explain that, I may sign this off. This is the whole point of the patch. Have a look at the related bugs, and what this patch is trying to prevent. If the variable contains a regex character (like parenthesis in the case of bug 22941, or $ in the case of bug 23006), then you need to surround your var with \Q\E, see man perlre). So the patch could have replaced grep {/^$var$/} @array with grep {/^\Q$var\E$/} @array But I preferred the other solution, I am sure you understand why :) > Everything else is a result of me seeing tcohen wanting to improve > readability of code. "unless grep" is better read as "if none". I am not tcohen :) The patch is a bugfix (updating the status). I would like to prevent other issues like the ones listed in see also. Replacing them with none, any or anything else will need more grey matter to make sure the changes will not introduce a regression. I am not against what you suggest but I think it should be done separately.
> This is the whole point of the patch. Have a look at the related bugs, and > what this patch is trying to prevent. If the variable contains a regex > character (like parenthesis in the case of bug 22941, or $ in the case of > bug 23006), then you need to surround your var with \Q\E, see man perlre). > > So the patch could have replaced > grep {/^$var$/} @array > with > grep {/^\Q$var\E$/} @array > > But I preferred the other solution, I am sure you understand why :) Yes, anything that requires an escaped solution adds a level of complexity and poor readability which makes it prone to breaking. > > Everything else is a result of me seeing tcohen wanting to improve > > readability of code. "unless grep" is better read as "if none". > > I am not tcohen :) > The patch is a bugfix (updating the status). I would like to prevent other > issues like the ones listed in see also. Replacing them with none, any or > anything else will need more grey matter to make sure the changes will not > introduce a regression. I am not against what you suggest but I think it > should be done separately. I agree with that, which is why I just wanted an explanation of the \Q \E thing. Thanks for that.
Comment on attachment 90823 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} Review of attachment 90823 [details] [review]: ----------------------------------------------------------------- These are the two places I will actually test, because everything else is a easy eyeball by comparison. ::: opac/opac-memberentry.pl @@ +77,4 @@ > > my @libraries = Koha::Libraries->search; > if ( my @libraries_to_display = split '\|', C4::Context->preference('PatronSelfRegistrationLibraryList') ) { > + @libraries = map { my $b = $_; my $branchcode = $_->branchcode; grep { $_ eq $branchcode } @libraries_to_display ? $b : () } @libraries; Super ugly code. There won't be scoping issues the $_? ::: opac/opac-search-history.pl @@ +70,4 @@ > @searches = map { $_->{type} ne $type ? $_ : () } @searches; > } > if ( @id ) { > + @searches = map { my $search = $_; ( grep { $_ eq $search->{id} } @id ) ? () : $_ } @searches; Super ugly code. Test for scoping issues.
Comment on attachment 90823 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} Review of attachment 90823 [details] [review]: ----------------------------------------------------------------- C4/Tags fails qa test tools.
Failing because of C4/Tags
The failure is: FAIL C4/Tags.pm FAIL forbidden patterns forbidden pattern: tab char (line 195) forbidden pattern: tab char (line 236) forbidden pattern: tab char (line 305) They existed before, not blocker.
Sorry, does not apply, conflicts in C4/Items.pm C4/OAI/Sets.pm C4/Search.pm admin/searchengine/elasticsearch/mappings.pl also a few more cases. C4/OAI/Sets.pm: if(0 == grep /^$value$/, @subfield_values) { C4/OAI/Sets.pm: if(0 < grep /^$value$/, @subfield_values) { misc/translator/translate: if ( $cmd ne 'create' and $lang and not grep( /^$lang$/, @{ $installer->{langs} } ) ) { t/db_dependent/Acquisition.t: unless grep ( /^$field$/, @$exp_fields ); t/db_dependent/Budgets.t: my $is_in_new_budgets = grep /^$budget->{budget_id}$/, @new_budget_ids; t/db_dependent/Budgets.t: my $is_in_old_budgets = grep /^$budget->{budget_id}$/, @old_budget_ids; t/db_dependent/Serials.t:my @arrived_missing = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? $_ : () } @serials; t/db_dependent/Serials.t:my @others = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? () : $_ } @serials; t/db_dependent/Serials.t:@arrived_missing = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? $_ : () } @serials; t/db_dependent/Serials.t:@others = map { my $status = $_->{status}; ( grep { /^$status$/ } qw( 2 4 41 42 43 44 5 ) ) ? () : $_ } @serials;
Created attachment 98701 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo!
Rebased. Remaining occurrences are in tests, should not be a problem.
It applies now, no (new) qa errors. Just a question: In C4/TAGS.pm you replace scalar grep {/^ $key $/x} @ok_fields with scalar grep { $_ eq $key } @ok_fields Is this correct? The spaces in '/^ $key $/x' are in the matching string (by way of /x), then perhaps scalar grep { $_ eq " $key " } @ok_fields is preferable.
the 'x' modifier allows whitespaces, so they must be explicitly provided by \w to be taken into account. So I think we are good here.
Created attachment 98707 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Looks correct, no errors.
(In reply to Jonathan Druart from comment #16) > the 'x' modifier allows whitespaces, so they must be explicitly provided by > \w to be taken into account. So I think we are good here. You are right!
Created attachment 98926 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Looks correct, no errors.
Please rebase - should be a quick one, but I messed it up :(
Created attachment 98960 [details] [review] Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Looks correct, no errors. Rebased.
(In reply to Katrin Fischer from comment #20) > Please rebase - should be a quick one, but I messed it up :( Rebased!
(In reply to Bernardo Gonzalez Kriegel from comment #22) > (In reply to Katrin Fischer from comment #20) > > Please rebase - should be a quick one, but I messed it up :( > > Rebased! Thx Bernardo!
Could we keep them from creeping back in with a new addition to the QA test tools?
Created attachment 98968 [details] [review] j# Attachment to Bug 23084 - Replace grep {^$var$} with grep {$var eq $_} Bug 23084: Replace grep {^$var$} with grep {$_ eq $var} We certainly faced 3 similar bugs due to this syntax: bug 23006, bug 22941 and bug 17526. To prevent other issues related to this syntax this patch suggests to replace them all in one go. Test plan: Confirm that the 2 syntaxes are similar Eyeball the patch and confirm that there is no typo! Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Looks correct, no errors. Rebased. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Nice work everyone! Pushed to master for 20.05
(In reply to Katrin Fischer from comment #24) > Could we keep them from creeping back in with a new addition to the QA test > tools? I did not manage to end up with something that would not raise too many false positives.
(In reply to Jonathan Druart from comment #27) > (In reply to Katrin Fischer from comment #24) > > Could we keep them from creeping back in with a new addition to the QA test > > tools? > > I did not manage to end up with something that would not raise too many > false positives. Thx for checking Jonathan. Wonder if this is important enough for a note in the coding guidelines? Our case with a 100 libraries using $ in their barcode might be a bit specific... but I also think the new form of writing this is nicer and more readable.
does not apply to 19.11.x. not backporting