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

Use send for choice operations #295

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 6, 2024

Pulled out of #189

This looks up the operation name up front and avoids the case

@kbrock kbrock added the enhancement New feature or request label Nov 6, 2024
@kbrock kbrock requested review from agrare and Fryguy as code owners November 6, 2024 16:51
@kbrock kbrock mentioned this pull request Nov 6, 2024
11 tasks
@agrare agrare merged commit cce673c into ManageIQ:master Nov 6, 2024
5 checks passed
@agrare
Copy link
Member

agrare commented Nov 6, 2024

Nice, this wasn't my favorite code to write :D thanks for cleaning it up

@agrare agrare self-assigned this Nov 6, 2024
@kbrock kbrock deleted the choice_rule_payload_validation-send branch November 6, 2024 19:34
@@ -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

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

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

Successfully merging this pull request may close these issues.

3 participants