Need string compare condition operator

[joshc-slac]

Current Behavior

In addition to issue of ConditionItem being a little too tied to caget #46, ConditionItem is currently only built to check int comparison. Python is so permissive this may well be okay if we always use the equal comparision operator when checking strings, this could be done much better.

Suggested Solution

• check if val isinstance of str, then do string compare
• require explicit serialized value: is_string_compare

[tangkong]

If we utilize ConditionOperator and map those to python's built-in operator we should be ok (operator.eq is ==, operator.ge is >=, etc)

We don't have to treat string equality differently here, as long as we don't invoke operators that don't make sense for strings (ge, lt, etc)

[tangkong]

py_trees uses a similar construct actually, that I was thinking of pulling in

ComparisonExpression, As used here

[joshc-slac]

I agree that the permissiveness of python can let us and py_trees get away with this. While this may not be a breaking issue I can very well see a world where this gives us the ability to shoot ourselves in the foot. i.e. a user copy pastes and only edits the value to be a string not integer leaving the operator as gt.

I think our system should be more principled than leaving it to the whims of python string comparison which may prove somewhat difficult to debug. While I agree one can call this a user configuration issue our system should anticipate these lowest order bugs. The overarching perspective: just because the language lets us get away with it doesn't mean we shouldn't do it the correct way

[ZLLentz]

Josh, is your point here that the options for sensible comparisons depends on the type? That is, non-numeric types should never be allowed to be compared using <, > and similar sorts of concerns?

[joshc-slac]

I should cut a small PR to make a StringComparrisonConditionItem or the like to make myself happy and close this ticket. We can use the equal operator as yall suggest under the hood... A further thought now is that when I was doing this live putting to a PV that takes an EPICS enum type with a string was being wonky; that thought doesnt necessarily go here but...

[ZLLentz]

Another approach might be to make BinaryConditionItem check the types and operator choice and raise on post-init if you've picked an incompatible one.

I think making another class is fine, but what we really are after is protecting people from making invalid comparisons in the first place, so it's probably the existing class that has a trap. This trap will remain even if we create a new item intended for strings.

Alternatively, we can add your new class and make stuff like BinaryConditionItem and BoundedConditionItem raise if we try to use strings or other nonsensical types for the bounds or the values. Maybe some combination of this stuff is needed.

Food for thought