PICARD-2486: Add text-based comparison script functions

[rdswift]

Summary

• This is a…
  • Bug fix
  • Feature addition
  • Refactoring
  • Minor / simple change (like a typo)
  • Other
• Describe this change in 1-2 sentences: Adds new text-based comparison scriping functions.

Problem

The current scripting comparison functions $gt(), $gte(), $lt() and $lte() operate on interger values only. This does not support things like date comparisons. See the discussion at https://community.metabrainz.org/t/how-set-date-to-minimum-of-recording-firstreleasedate-and-originaldate/585648 and https://community.metabrainz.org/t/feature-request-numeric-functions-min-and-max-in-picards-scripting-language/585669 for more information.

• JIRA ticket (optional): PICARD-2486

Solution

Adds four new text-based comparison scripting functions $textlt(), $textlte(), $textgt() and $textgte(), plus text-based $textmin() and $textmax() functions.

Adds an optional argument to the existing $lt(), $lte(), $gt() and $gte() functions to specify whether the arguments should be processed as int, float, text (case-sensitive text) or nocase (case-insensitive text). If no argument is provided the comparison will be processed as int to maintain backward compatibility.

Also adds two new functions $min() and $max(), with the type of processing specified as one of int, float or text (case-sensitive text) as the first argument.

Action

I assume that, if accepted, these changes would be applied to Picard v3.0 (or v2.9 if there is such a release). If accepted for other than v3.0 then the function description docstrings will need to be updated accordingly.

I wasn't sure which branch to merge into, so I used the master branch. We can change the target if necessary.

If accepted, the Picard documentation will need to be updated.

[rdswift]

For the sake of completeness, should there also be integer-based $min() and $max() functions, even though I can't think of a use case?

[zas]

I'm a bit bothered by introduction of another full set of new comparison functions for text and I wonder if we should handle text type in existing comparison functions instead.
Type of comparison could be specified using a third parameter (this family of functions only accepts 2 parameters).

What I'm thinking about is something like:

$lt(1, 2) -> $lt(1, 2) or $lt(1, 2, int)
$textlt(a, b) -> $lt(a, b, text)

@phw, @rdswift : what do you think?

[rdswift]

@phw, @rdswift : what do you think?

I was actually going to suggest that as an alternative, so I'm good with it.

[rdswift]

In a related PR @zas commented:

I wonder if we could extend this to eq(), ne() and not().

(Copied here to keep the discussion in one place.)

[rdswift]

I wonder if we could extend this to eq(), ne() and not().

We could, but it might be a bit odd because (as you pointed out in IRC) the default processing type is different for these functions.

[phw]

Type of comparison could be specified using a third parameter (this family of functions only accepts 2 parameters).

I agree that another set of functions becomes difficult. And as a user I'd really expect this to do the right thing. Maybe we could even do an auto mode (in addition to the explicit int and text modes) that does int comparison if both values are numeric strings, otherwise text comparison?

[rdswift]

Maybe we could even do an auto mode (in addition to the explicit int and text modes) that does int comparison if both values are numeric strings, otherwise text comparison?

That sounds like a good idea. I can take a look at adding that. Should "auto" become the default? Even though this new default would be different, I don't think it would break any scripts.

[rdswift]

I've implemented the auto processing type option, which first uses int or float in that order if all arguments pass the type, falling back to text if both int and float raise exceptions. For now, I've made this the default if no processing option is specified. Is this okay?

Also, there are two different text comparison types available: text is a case-sensitive compare, and nocase is a case-insensitive compare. The fallback for auto is currently text, but I'm wondering if it should be changed to nocase instead because that is the most flexible comparison method. Any preference on this?