560009 - Use firstidx from List::MoreUtils instead of lsearch

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

Most of the places that we use lsearch should simply be using grep. However, places that actually need to know the index of an item can use firstidx, from List::MoreUtils, instead.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, this totally removes lsearch from Bugzilla and replaces it either with grep or firstidx, as appropriate.

I tested that all the files compile, but I haven't actually tested their functionality.

This is mostly a pretty straightforward patch. timello--would you like to start helping out a bit here and there with reviews? I think this'd be a nice patch to help out with, if you could. It's a little large, but it's pretty simple in each of its pieces.

Comment 2

[Tiago Mello [:timello]]

Sure! Thanks for the confidence.

Comment 3

[Tiago Mello [:timello]]

Comment on attachment 439678 [details] [diff] [review]
v1

The code looks good. I've applied it and tested as much as possible and functions grep and firstidx do the same work as the old lsearch.

Nits:

 * I see the most of your grep format is:
  grep {...} @list;
even within a 'if', however there are some parts you use the format:
  grep(..., @list);
I'm wondering if we could use the same format in all the patch.
 * We could change the name of the template macro to something like index_of (your suggestion) to keep name readable and consistence with the rest of the code. We could do it in a separated bug.

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
>  * I see the most of your grep format is:
>   grep {...} @list;
> even within a 'if', however there are some parts you use the format:
>   grep(..., @list);
> I'm wondering if we could use the same format in all the patch.

  I'd like to, but there are precedence problems, so I used grep() when I needed to have the right precedence.

>  * We could change the name of the template macro to something like index_of
> (your suggestion) to keep name readable and consistence with the rest of the
> code. We could do it in a separated bug.

  Yeah, that's a good idea, and we'll do it separately.

Comment 5

[Max Kanat-Alexander]

Thank you so much for the review, timello!! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla.pm
modified attachment.cgi
modified buglist.cgi
modified editflagtypes.cgi
modified enter_bug.cgi
modified process_bug.cgi
modified report.cgi
modified showdependencygraph.cgi                                               
modified Bugzilla/Bug.pm
modified Bugzilla/CGI.pm
modified Bugzilla/Field.pm
modified Bugzilla/Template.pm
modified Bugzilla/User.pm
modified Bugzilla/Util.pm
modified Bugzilla/Config/Common.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Migrate/Gnats.pm                                             
modified contrib/bzdbcopy.pl
modified t/007util.t
Committed revision 7138.

Comment 6

[Max Kanat-Alexander]

Added to the release notes in bug 604256.