feat(condition): Update conditions to support source and target key comparisons

Mallika05 commented 2 months ago

Description

Updated the following conditions to support comparisons between source_key and target_key values.

number_equal_to
number_greater_than
number_less_than
string_equal_to
string_greater_than
string_less_than

Example:

sub.condition.number.less_than( settings={object: {source_key: 'a', target_key: 'z'}} ) sub.condition.string.less_than( settings={object: {source_key: 'a', target_key: 'z'}} )

Motivation and Context

This adds flexibility to accept both static values or target_key whichever is provided in the config. Static value is ignored if target_key is present in the condition. This solves the issue discussed here: https://github.com/brexhq/substation/issues/64.

How Has This Been Tested?

Added unit tests in respective files.

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[ ] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

Mallika05 commented 2 months ago

#214 👈
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @Mallika05 and the rest of your teammates on Graphite

jshlbrd commented 2 months ago

The additions look good but I have an ask for changes to each function, it's slightly better to do this:

    compare := insp.conf.Value

    if insp.conf.Object.SourceKey == "" {
        f, err := strconv.ParseFloat(string(msg.Data()), 64)
        if err != nil {
            return false, err
        }

        return insp.match(f, compare), nil
    }

    target := msg.GetValue(insp.conf.Object.TargetKey)
    if target.Exists() {
        compare = target.Float()
    }

    v := msg.GetValue(insp.conf.Object.SourceKey)
    return insp.match(v.Float(), compare), nil

Instead of this:

    compare := insp.conf.Value

    target := msg.GetValue(insp.conf.Object.TargetKey)
    if target.Exists() {
        compare = target.Float()
    }

    if insp.conf.Object.SourceKey == "" {
        f, err := strconv.ParseFloat(string(msg.Data()), 64)
        if err != nil {
            return false, err
        }

        return insp.match(f, compare), nil
    }

    v := msg.GetValue(insp.conf.Object.SourceKey)
    return insp.match(v.Float(), compare), nil

If conf.Object.SourceKey is empty, then the condition is evaluating the message's content directly, so it can't do a comparison between two different values (e.g. {"foo":1.0} shouldn't be compared to 1.0 because it will always be false). We'll get a little more performance by accessing the TargetKey after that statement than before it.

If someone were to configure these functions like that, then that would be a user error (identical to this one).

brexhq / substation