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.

Sign up for GitHub

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

Use send for choice operations #295

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Use send for choice operations #295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 32 additions & 37 deletions lib/floe/workflow/choice_rule/data.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ class Data < Floe::Workflow::ChoiceRule
# e.g.: (String)(LessThan)(Path), (Numeric)(GreaterThanEquals)()
OPERATION = /^(#{(TYPES - %w[Null Present]).join("|")})(#{COMPARES.join("|")})(Path)?$/.freeze

attr_reader :variable, :compare_key, :type, :compare_predicate, :path
attr_reader :variable, :compare_key, :operation, :type, :compare_predicate, :path

def initialize(_workflow, _name, payload)
super
Expand All @@ -25,39 +25,7 @@ def true?(context, input)

lhs = variable_value(context, input)
rhs = compare_value(context, input)

case compare_key
when "IsNull" then is_null?(lhs, rhs)
when "IsNumeric" then is_numeric?(lhs, rhs)
when "IsString" then is_string?(lhs, rhs)
when "IsBoolean" then is_boolean?(lhs, rhs)
when "IsTimestamp" then is_timestamp?(lhs, rhs)
when "StringEquals", "StringEqualsPath",
"NumericEquals", "NumericEqualsPath",
"BooleanEquals", "BooleanEqualsPath",
"TimestampEquals", "TimestampEqualsPath"
lhs == rhs
when "StringLessThan", "StringLessThanPath",
"NumericLessThan", "NumericLessThanPath",
"TimestampLessThan", "TimestampLessThanPath"
lhs < rhs
when "StringGreaterThan", "StringGreaterThanPath",
"NumericGreaterThan", "NumericGreaterThanPath",
"TimestampGreaterThan", "TimestampGreaterThanPath"
lhs > rhs
when "StringLessThanEquals", "StringLessThanEqualsPath",
"NumericLessThanEquals", "NumericLessThanEqualsPath",
"TimestampLessThanEquals", "TimestampLessThanEqualsPath"
lhs <= rhs
when "StringGreaterThanEquals", "StringGreaterThanEqualsPath",
"NumericGreaterThanEquals", "NumericGreaterThanEqualsPath",
"TimestampGreaterThanEquals", "TimestampGreaterThanEqualsPath"
lhs >= rhs
when "StringMatches"
lhs.match?(Regexp.escape(rhs).gsub('\*', '.*?'))
else
raise Floe::InvalidWorkflowError, "Invalid choice [#{compare_key}]"
end
send(operation, lhs, rhs)
end

private
Expand Down Expand Up @@ -112,26 +80,53 @@ def is_timestamp?(value, predicate = true)
# rubocop:enable Naming/PredicateName
# rubocop:enable Style/OptionalBooleanParameter

def equals?(lhs, rhs)
Copy link
Member

Choose a reason for hiding this comment

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

This method name scares me a little because it shadows a common Ruby method for object equality.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fryguy Still thinking about this comment

We define @next and @end because that is the name of the parameter.

Here, we define equals? because it is derived from the operation Equals.

Do we want to add an string to the beginning of the phrase to have methods like opEquals? ?

Copy link
Member

@Fryguy Fryguy Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

+1 for a op prefix - op_equals? would work for me too.

next and end are also good examples of interface names that scare me because they collide with Ruby keywords. The difference there is that the keywords are bare, so they only affect scoped usage. For example, an external user of Map would do map.next or map.end, which has a defined receiver, so it doesn't collide, but a locally scoped method might call next bare, and then you have an ambigous receiver vs keyword.

Note that next also can collide with ruby debug's next command - no idea what would happen there.

Copy link
Member

Choose a reason for hiding this comment

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

Actually i just played around with locally, and you can't even call next without a receiver

# next_end.rb
class Foo
  def next
    puts "next"
  end

  def end
    puts "end"
  end
end

without a receiver:

class Foo
  def call_next_no_receiver
    next
  end
end
(irb):2:in `require_relative': --> /Users/jfrey/dev/test/next_end.rb
Invalid next
   1  class Foo
  10    def call_next_no_receiver
> 11      next
  12    end
  13  end

/Users/jfrey/dev/test/next_end.rb:11: Invalid next (SyntaxError)
	from (irb):2:in `<main>'
	from <internal:kernel>:187:in `loop'
	from /Users/jfrey/.rubies/ruby-3.3.6/lib/ruby/gems/3.3.0/gems/irb-1.13.1/exe/irb:9:in `<top (required)>'
	from /Users/jfrey/.gem/ruby/3.3.6/bin/irb:25:in `load'
	from /Users/jfrey/.gem/ruby/3.3.6/bin/irb:25:in `<main>'

making it syntactically correct by using next in a loop

class Foo
  def call_next_no_receiver
    5.times do
      next
      puts "skipped"
    end
  end

  def call_next_w_receiver
    5.times do
      self.next
      puts "not skipped"
    end
  end
end

see that you must use a receiver or the keyword takes precedence

irb(main):001> require_relative './next_end'
=> true
irb(main):002> Foo.new.call_next_no_receiver
=> 5
irb(main):003> Foo.new.call_next_w_receiver
next
not skipped
next
not skipped
next
not skipped
next
not skipped
next
not skipped
=> 5

lhs == rhs
end

def lessthan?(lhs, rhs)
lhs < rhs
end

def greaterthan?(lhs, rhs)
lhs > rhs
end

def lessthanequals?(lhs, rhs)
lhs <= rhs
end

def greaterthanequals?(lhs, rhs)
lhs >= rhs
end

def matches?(lhs, rhs)
lhs.match?(Regexp.escape(rhs).gsub('\*', '.*?'))
end

# parse the compare key at initialization time
def parse_compare_key
payload.each_key do |key|
# e.g. (String)(GreaterThan)(Path)
if (match_values = OPERATION.match(key))
@compare_key = key
@type, _operator, @path = match_values.captures
@type, operator, @path = match_values.captures
@operation = "#{operator.downcase}?".to_sym
@compare_predicate = parse_predicate(type)
break
end
# e.g. (Is)(String)
if TYPE_CHECK.match?(key)
if (match_value = TYPE_CHECK.match(key))
@compare_key = key
_operator, type = match_value.captures
# type: nil means no runtime type checking.
@type = @path = nil
@operation = "is_#{type.downcase}?".to_sym
@compare_predicate = parse_predicate("Boolean")
break
end
end
parser_error!("requires a compare key") unless compare_key
parser_error!("requires a compare key") if compare_key.nil? || operation.nil?
end

# parse predicate at initilization time
Expand Down
7 changes: 6 additions & 1 deletion spec/workflow/choice_rule_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,15 @@
end

context "with non-path Compare Key" do
let(:choices) { [{"Variable" => "$foo", "StringEqualsPath" => "wrong", "Next" => "FirstMatchState"}] }
let(:choices) { [{"Variable" => "$.foo", "StringEqualsPath" => "wrong", "Next" => "FirstMatchState"}] }
it { expect { workflow }.to raise_exception(Floe::InvalidWorkflowError, "States.Choice1.Choices.0.Data field \"StringEqualsPath\" value \"wrong\" Path [wrong] must start with \"$\"") }
end

context "with invalid Compare Key" do
let(:choices) { [{"Variable" => "$.foo", "IntegerMatchPath" => "$.foo", "Next" => "FirstMatchState"}] }
it { expect { workflow }.to raise_exception(Floe::InvalidWorkflowError, "States.Choice1.Choices.0.Data requires a compare key") }
end

context "with second level Next (Not)" do
let(:choices) { [{"Not" => {"Variable" => "$.foo", "StringEquals" => "bar", "Next" => "FirstMatchState"}, "Next" => "FirstMatchState"}] }

Expand Down