Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix #1512] add not in comparison into Trigger Agent #1545

Merged
merged 1 commit into from
Jun 18, 2016
Merged

[fix #1512] add not in comparison into Trigger Agent #1545

merged 1 commit into from
Jun 18, 2016

Conversation

andrey-yantsen
Copy link
Contributor

Add not in comparison for doing != on arrays. Also warning about != and arrays was added.
Fixes #1512.

@@ -11,7 +11,7 @@ class TriggerAgent < Agent

The `type` can be one of #{VALID_COMPARISON_TYPES.map { |t| "`#{t}`" }.to_sentence} and compares with the `value`. Note that regex patterns are matched case insensitively. If you want case sensitive matching, prefix your pattern with `(?-i)`.

The `value` can be a single value or an array of values. In the case of an array, if one or more values match then the rule matches.
The `value` can be a single value or an array of values (all values must be stored as strings). In the case of an array, if one or more values match then the rule matches. *Warning*: avoid using `field!=value` with arrays, you should use `not in` instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe "(in which case, all items in the array must be strings)"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More complete re-write:

The value can be a single value or an array of values. In the case of an array, all items must be strings, and if one or more values match, then the rule matches. Note: avoid using field!=value with arrays, you should use not in instead.

@cantino
Copy link
Member

cantino commented Jun 14, 2016

Thanks @andrey-yantsen, would you mind updating the spec too?

@andrey-yantsen
Copy link
Contributor Author

Updating spec was a great idea :)
Give me an example for proper rewriting of rule_values.any? and I'll squash commits into one.

@cantino
Copy link
Member

cantino commented Jun 16, 2016

Maybe just this?

rule_values.any? do |rule_value|
  if rule['type'] == "regex"
    value_at_path.to_s =~ Regexp.new(rule_value, Regexp::IGNORECASE)
  elsif rule['type'] == "!regex"
    value_at_path.to_s !~ Regexp.new(rule_value, Regexp::IGNORECASE)
  elsif rule['type'] == "field>value"
    value_at_path.to_f > rule_value.to_f
  elsif rule['type'] == "field>=value"
    value_at_path.to_f >= rule_value.to_f
  elsif rule['type'] == "field<value"
    value_at_path.to_f < rule_value.to_f
  elsif rule['type'] == "field<=value"
    value_at_path.to_f <= rule_value.to_f
  elsif rule['type'] == "field==value"
    value_at_path.to_s == rule_value.to_s
  elsif rule['type'] == "field!=value"
    value_at_path.to_s != rule_value.to_s
  elsif rule['type'] == 'in'
    rule_values.include?(value_at_path.to_s)
  elsif rule['type'] == 'not in'
    !rule_values.include?(value_at_path.to_s)
  else
    raise "Invalid type of #{rule['type']} in TriggerAgent##{id}"
  end
end

I added in there too.

@andrey-yantsen
Copy link
Contributor Author

I can not agree with it :) It's quite strange to compare entire list inside rule_values.any? — according to your suggestion exactly same operation will be performed for each element of the list. Also — .include? operation should actually be performed for any ==.

@cantino
Copy link
Member

cantino commented Jun 16, 2016

You're right, I misunderstood.

Want to add in next to your not in option at the top of the if?

@andrey-yantsen
Copy link
Contributor Author

Actually in is quite strange operation because it is exactly the same thing as field==value. I've added it, but I'm confused a little by this decision :)

@cantino
Copy link
Member

cantino commented Jun 16, 2016

Yea, that's a good point. I'm tired this evening. We don't need it :)

@andrey-yantsen
Copy link
Contributor Author

Ok :)
I've squashed the commits into one, check it once again please.

@cantino cantino merged commit 3b4b4f3 into huginn:master Jun 18, 2016
@cantino
Copy link
Member

cantino commented Jun 18, 2016

Thanks @andrey-yantsen, works great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants