277622 - Move DiffStrings() out of globals.pl into Bugzilla/Util.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

This certainly is an interesting function... I'm surprised that there's nothing
built-in to Perl or any of the modules that we require that already does this.

Anyhow, it should be fairly easy to move this into Util.pm, and just rename it
to have the lowercase_underscore naming scheme. It's only used in three places.

Comment 1

[Max Kanat-Alexander]

Attachment: Simple Patch

Here's a basic patch that does the job. I just moved the code and re-named
things, and made sure that everybody was using Bugzilla::Util who needed to.

I tested the code to make sure that it works.

Comment 2

[Vlad Dascalu]

Comment on attachment 170722 [details] [diff] [review]
Simple Patch

>> Returns an array

It returns a list. See http://japhy.perlmonk.org/articles/pm/2000-02.html#2 for
a difference between lists and arrays, especially the "Lists vs. Arrays"
section and the "Return Values" section (where A() returns a lists and B()
returns an array, in non-scalar context).

+Takes two comma- or space-separated strings and returns what values

I'd say:

+Takes two strings with comma- or space-separated values

or something similar, because from the comment someone might understand that
the strings themselves are space-separated. There is no mention of the fact
that those strings contain values separated by comma or space until much later
in the sentence.

NIT: we might also say that the items returned are always comma separated.

Comment 3

[Vlad Dascalu]

Comment on attachment 170722 [details] [diff] [review]
Simple Patch

+... added items.

NIT: we could use the same terminology (either values or items) to be
consisntant, but that's a nit also.

Comment 4

[Max Kanat-Alexander]

Attachment: Version 2

Wow, thank you for that very helpful review. :-) I was confused about the
documentation, myself -- I'm glad that now we'll have hopefully clear API
documentation. (Now *I* also understand what the function is supposed to do.
:-) )

Comment 5

[Dave Miller [:justdave]]

Comment on attachment 170795 [details] [diff] [review]
Version 2

>+compared to the old one. Returns a list, where the first entry is a scalar
>+containing removed items, and the second entry is a scalar containing added
>+items.

Nit: Returns a list, where the first entry is a reference to a list containing
the removed items, and the second entry is a reference to a list containing the
added items.

Comment 6

[Max Kanat-Alexander]

Attachment: Version 3

OK, I corrected justdave's (very valid) nit.

Comment 7

[Dave Miller [:justdave]]

(In reply to comment #6)
> Created an attachment (id=170796) [edit]
> Version 3
> 
> OK, I corrected justdave's (very valid) nit.

Ummm, errr....   Oops.

Take version 2.  I didn't read the patch close enough.

It returns strings with "comma+space" separated human-readable lists in those
strings.

Comment 8

[Dave Miller [:justdave]]

(In reply to comment #7)

> It returns strings with "comma+space" separated human-readable lists in those
> strings.

Having it return references to lists might actually make the function a little
cleaner, and allow the caller to be more flexible with the results...  On the
other hand, everyplace currently using it is using it with the intention of
producing an entry in the activity table, which wants it as a string anyway
(currently anyway -- there's another bug somewhere suggesting that we change
that, and have a separate entry for each added and removed item instead of comma
lists, to make searches more accurate)

Probably better to leave it like it is for now, and we can change it to return
lists as part of the above bug.

Comment 9

[Max Kanat-Alexander]

Comment on attachment 170796 [details] [diff] [review]
Version 3

OK, then this version is incorrect. :-)

Comment 10

[Max Kanat-Alexander]

Comment on attachment 170795 [details] [diff] [review]
Version 2

And this version needs a very brief review. :-)

Comment 11

[Vlad Dascalu]

Targeting bug to 2.20 since the 2.20 feature freeze was canceled.

Comment 12

[Vlad Dascalu]

Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.281; previous revision: 1.280
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.223; previous revision: 1.222
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.28; previous revision: 1.27
done
Checking in Bugzilla/Util.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Util.pm,v  <--  Util.pm
new revision: 1.15; previous revision: 1.14
done