Handle two hash arguments to lazy_find

[agrare]

One of the ways of calling lazy_find involves passing a hash for the manager_uuid as well as for the options.

Example:

inventory_object.counterpart = persister.cross_link_vms.lazy_find({:uid_ems => host.instance_id}, {:ref => :by_uid_ems})

This was causing an ArgumentError since lazy_find was only expecting one positional argument.

Follow-up to https://github.com/ManageIQ/inventory_refresh/pull/112

[Fryguy]

While I'm fine with this - I thought the point of the keyword args was to avoid the second Hash. That is, you should be able to just call without the braces or if in a hash, use **opts?

[agrare]

I thought the point of the keyword args was to avoid the second Hash. That is, you should be able to just call without the braces or if in a hash, use **opts?

Oh I thought it was to handle the: lazy_find(:ems_ref => "vm-1") case (which it does well) but it sees ({hash}, {hash}) as two positional arguments.

So using ruby3:

>> RUBY_VERSION
=> "3.0.4"

def lazy_find(manager_uuid = nil, ref: :manager_ref, key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash)
  return if manager_uuid.nil? && manager_uuid_hash.blank?

  raise ArgumentError, "only one of manager_uuid or manager_uuid_hash must be passed" unless !!manager_uuid ^ !!manager_uuid_hash.present?

  puts "manager_uuid 0: #{manager_uuid} manager_uuid_hash: #{manager_uuid_hash}"
  manager_uuid ||= manager_uuid_hash
  puts "manager_uuid: #{manager_uuid} ref: #{ref} key: #{key} default: #{default}"
end

>> lazy_find(:ems_ref => "vm-1")
manager_uuid 1:  manager_uuid_hash: {:ems_ref=>"vm-1"}
manager_uuid: {:ems_ref=>"vm-1"} ref: manager_ref key:  default: 

>> lazy_find({:ems_ref => "vm-1"}, {:ref => :ems_ref})
(irb):34:in `lazy_find': wrong number of arguments (given 2, expected 0..1) (ArgumentError)
    from (irb):41:in `<main>'
    from /usr/lib/ruby/gems/3.0.0/gems/irb-1.3.5/exe/irb:11:in `<top (required)>'
    from /usr/bin/irb3.0:23:in `load'
    from /usr/bin/irb3.0:23:in `<main>'

[Fryguy]

Right, can you try the following to fill out the use cases:

>> lazy_find({:ems_ref => "vm-1"}, :ref => :ems_ref)
>> lazy_find({:ems_ref => "vm-1"}, **{:ref => :ems_ref})

[Fryguy]

I guess the question is do we want to handle the 2 hash argument use case, or just force the caller either un-hash it or splat it.

[agrare]

Oh yeah if you **{:ref => } that works but I thought we were trying to not require changing the callers.

[Fryguy]

Oh yeah if you **{:ref => } that works but I thought we were trying to not require changing the callers.

hmmm...maybe? If we're doing it for compatibility, or to temporarily avoid changing code, I think that's ok. However, ultimately what we're doing here feels like writing our own second, custom, form of keyword args, which feels like we're hacking around Ruby, and that makes me think we should change the callers to use Ruby properly.

[agrare]

If we're doing it for compatibility, or to temporarily avoid changing code, I think that's ok.

Yeah we can change all the callers (actually had started that already https://github.com/ManageIQ/manageiq-providers-amazon/pull/786) but I thought your point of https://github.com/ManageIQ/inventory_refresh/pull/97#issuecomment-1011121116 and the *manager_uuid_hash was to not change the callers

However, ultimately what we're doing here feels like writing our own second, custom, form of keyword args, which feels like we're hacking around Ruby, and that makes me think we should change the callers to use Ruby properly.

Well it was more for compatibility with 2.7 and 3.0 when calling using the lazy_find("vm-1", {:ref => :uid_ems}) format which worked with kwargs on 2.7

[Fryguy]

Yeah we can change all the callers (actually had started that already https://github.com/ManageIQ/manageiq-providers-amazon/pull/786) but I thought your point of https://github.com/ManageIQ/inventory_refresh/pull/97#issuecomment-1011121116 and the *manager_uuid_hash was to not change the callers

What I was trying to do in #97 was to find a way to change the signature temporarily to allow both signatures, so we could avoid having to merge across multiple repos simultaneously with releasing a gem, which is a coordination pain. My other concern was that we'd miss finding a caller, causing bugs, so it would give us a place to tack in deprecation messages letting the code tell us where the callers are. Later after we change the callers, we can remove the temporary signature. If that's essentially what this PR does, then I am :+1: .

Honestly, I thought #97 was already merged, which is adding to my confusion. (I thought we were already at the "change the callers" phase of that plan). I see now that #112 was the implementation of that temporary signature, and this is really a fix on top of that, so I am :+1:

Well it was more for compatibility with 2.7 and 3.0 when calling using the lazy_find("vm-1", {:ref => :uid_ems}) format which worked with kwargs on 2.7

I always forget about that Ruby 2.7 thing - I shouldn't because it's part of the reason 3.0 came out in the first place :(

[agrare]

Honestly, I thought https://github.com/ManageIQ/inventory_refresh/pull/97 was already merged, which is adding to my confusion. (I thought we were already at the "change the callers" phase of that plan)

Yeah I took your suggestion and started running with it in https://github.com/ManageIQ/inventory_refresh/pull/112 I only found that this case was failing when starting the cross-repo testing with ruby3. I think we can close #97 now that #112 is in.

I like adding a deprecation warning if opts is present I can do that in this PR.

[kbrock]

I agree with not wanting to change all callers and release a gem at the same time. It does feel like we're bending over backwards a little too much.

So the current thought is this kwargs / merge hashes kind of thing is temporary? Do we want to comment it as such so our future selves know which parts of the code are good and which parts are hacks?

[miq-bot]

Checked commits https://github.com/agrare/inventory_refresh/compare/1168e0821e3b90c7a2fd42e8d9e2a8afcc48967a~...e599e73ec5244b83bce38dcb3be90855b0a02f4a with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint 2 files checked, 0 offenses detected Everything looks fine. :cookie: