[WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 #97

jrafanie · 2022-01-07T17:37:34Z

WIP because this is a branch direct PR... I'm not sure why master and 0.2.z diverged. See below

This is for ruby 3 support which requires we clarify when we're passing a hash or keyword arguments as the last argument in the method definition and invocation.

This PR uses 0.2.z as the baseline since rails 6 support is not on master but is on 0.2.z but master has other changes so the two branches have diverged.

If needed, we can apply missing PRs from 0.2.z onto master and apply this change there instead.

Let me know.

agrare · 2022-01-07T18:29:22Z

I'm not sure why master and 0.2.z diverged.

We branched 0.2.z for insights/topology work that would have broken/required significant updates to ManageIQ (specifically requiring concurrent_safe batch saving for every table, and the different method of sql batch deletion). Still a TODO of mine to get ManageIQ back on to master here but we're not ready for that yet.

agrare · 2022-01-07T18:30:00Z

spec/persister/finders_spec.rb

@@ -19,7 +19,7 @@
  it "raises an exception when relation object is needed, but something else is provided" do
    expected_error = "Wrong index for key :vm_or_template, the value must be of type Nil or InventoryObject or InventoryObjectLazy, got: not_allowed_string"
    expect do
-      persister.hardwares.lazy_find(:vm_or_template => "not_allowed_string")
+      persister.hardwares.lazy_find({:vm_or_template => "not_allowed_string"})


This is going to require a lot of changes in MIQ if we have to change the callers...

True, but this is only because lazy_find accepts a hash as the first argument and optional keyword arguments for the rest. Maybe I'm naive but I believe we generally used options hashes as the last argument and didn't really adopt keyword arguments generally.

The changes in core here are pretty small but it's possible we're not really exercising much of the code and we could find others like this in other repositories.

I think what's confusing is that the first parameter is named manager_uuid, when in fact it accepts a hash.

inventory_refresh/lib/inventory_refresh/inventory_collection/index/proxy.rb

Line 89 in 0b5bb8d

def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false)

manager_ref or reference would be more accurate, and if manager_ref is only a single column you can just pass a string to lazy_find for simplicity

I wonder if it's better to make that a keyword arg also, so something like:

def lazy_find(manager_ref:, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false) ############## persister.hardwares.lazy_find(manager_ref: {:vm_or_template => "not_allowed_string"})

I wonder if it's better to make that a keyword arg also, so something like:

Maybe? Might just be lazy but vms.lazy_find("vm-123") is pretty nice compared with having to pass vms.lazy_find(manager_ref: "vm-123")

If we're doing a major version bump anyway...these methods have way too many options haha
Almost all of these could be handled automatically, the only one that can't is :default but I'm not sure if that's used currently

yeah agreed...the signatures here have always been pretty nasty.

Is the first parameter to lazy_find optional?
If it is non-optional, then I think ruby will be smart enough to know it is the first parameter and not part of the kwargs.

Other than that, think this is the new style of ruby and the path we need to take

Is the first parameter to lazy_find optional?

No, it's a required positional argument.

inventory_refresh/lib/inventory_refresh/inventory_collection/index/proxy.rb

Line 89 in 0b5bb8d

def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false)

EDIT by @Fryguy: my suggestion was copy-pasted as opposed to the real signature.

kbrock

if first param is already non-optional, (or can't be converted to a non-optional parameter) then LGTM

kbrock · 2022-01-11T00:50:53Z

spec/persister/finders_spec.rb

@@ -19,7 +19,7 @@
  it "raises an exception when relation object is needed, but something else is provided" do
    expected_error = "Wrong index for key :vm_or_template, the value must be of type Nil or InventoryObject or InventoryObjectLazy, got: not_allowed_string"
    expect do
-      persister.hardwares.lazy_find(:vm_or_template => "not_allowed_string")
+      persister.hardwares.lazy_find({:vm_or_template => "not_allowed_string"})


Is the first parameter to lazy_find optional?
If it is non-optional, then I think ruby will be smart enough to know it is the first parameter and not part of the kwargs.

Other than that, think this is the new style of ruby and the path we need to take

Fryguy · 2022-01-11T20:08:05Z

EDIT: Ignore these examples, because they are wrong...see #97 (comment)

old, wrong examples

Perhaps not for this PR, but to make it less clunky-looking, it might be worth it to use the double splat to our advantage and preserve the way callers originally called this. So, before this PR we had:

def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false) 

# callers
lazy_find("vm-1")
lazy_find(:ems_ref => ems_ref)
lazy_find(:name => "name", :ref => :by_name)

and after this PR we have

  lazy_find("vm-1")
- lazy_find(:ems_ref => ems_ref})
+ lazy_find({:ems_ref => ems_ref})
- lazy_find(:name => "name", :ref => :by_name)
+ lazy_find({:name => "name"}, :ref => :by_name)

However, if we change the signature instead to this:

- def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false) 
+ def lazy_find(manager_uuid = nil, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash) 
+   manager_uuid ||= manager_uuid_hash

then I think we can keep the same original signature and no warnings. We may also want to add a few more ArgumentError checks that the user can't pass both nor neither of manager_uuid and manager_uuid_hash. We'd end up with this:

def lazy_find(manager_uuid = nil, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash) 
  raise ArgumentError, "only one of manager_uuid or manager_uuid_hash must be passed" unless !!manager_uuid ^ !!manager_uuid_hash.present?
  manager_uuid ||= manager_uuid_hash
  
# callers  (now same as the original callers, so no changes)
lazy_find("vm-1")
lazy_find(:ems_ref => ems_ref)
lazy_find(:name => "name", :ref => :by_name)

jrafanie · 2022-01-11T21:27:13Z

However, if we change the signature instead to this:

- def lazy_find(manager_uuid, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false) 
+ def lazy_find(manager_uuid = nil, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash) 
+   manager_uuid ||= manager_uuid_hash

then I think we can keep the same original signature and no warnings. We may also want to add a few more ArgumentError checks that the user can't pass both nor neither of manager_uuid and manager_uuid_hash. We'd end up with this:

def lazy_find(manager_uuid = nil, ref: primary_index_ref, key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash) 
  raise ArgumentError, "only one of manager_uuid or manager_uuid_hash must be passed" unless !!manager_uuid ^ !!manager_uuid_hash.present?
  manager_uuid ||= manager_uuid_hash
  
# callers
lazy_find("vm-1")
lazy_find(:ems_ref => ems_ref)
lazy_find(:name => "name", :ref => :by_name})

Hmm, as a newbie to this method, I had no idea that the positional argument manager_uuid was or could be a hash and so I have a hard time ever understanding the callers, especially the last example... is there an accidental trailing } in there?.

Making the positional argument an explicit hash seems better in the short term. It feels like fixing the method signature so variables match their contents and possibly go all kwargs or all hash would seem to be a more clear way forward.

The last one is just confusing to me.

agrare · 2022-01-11T21:29:10Z

The last one is just confusing to me.

I think that last one is supposed to be lazy_find(:name => "name", {:ref => :by_name})

Fryguy · 2022-01-12T14:48:21Z

@jrafanie

is there an accidental trailing } in there?.

Yes, fixed my example.

The last one is just confusing to me.

Take another look...the general idea is change the signature (only a small localized change) so that none of the callers have to change.

Fryguy · 2022-01-12T15:01:08Z

On relooking, maybe my examples are wrong...sorry about that. I was pulling some from the specs not realizing that the specs had some intentionally wrong examples 😩 Let me try to reformulate the examples, because I think the idea still has merit.

Fryguy · 2022-01-12T15:22:47Z

Updated example using RUBYOPT='-w' irb on Ruby 2.7

Before:

def lazy_find_orig(manager_uuid, ref: "default_ref", key: nil, default: nil, transform_nested_lazy_finds: false)
  puts manager_uuid, ref
end

def lazy_find_prop(manager_uuid = nil, ref: "default_ref", key: nil, default: nil, transform_nested_lazy_finds: false, **manager_uuid_hash)
  manager_uuid ||= manager_uuid_hash
  puts manager_uuid, ref
end

lazy_find_orig("vm-1")
# => vm-1
# => default_ref
lazy_find_prop("vm-1")
# => vm-1
# => default_ref

lazy_find_orig(:ems_ref => 1)
# => (pry):5: warning: Passing the keyword argument as the last hash parameter is deprecated
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:ems_ref=>1}
# => default_ref
lazy_find_prop(:ems_ref => 1)
# => {:ems_ref=>1}
# => default_ref

# NOTE^: This is fixed with the proposal such that we don't have to change any callers, and
#        this is a large percentage of the changes in this PR.

lazy_find_orig({:name => "name"}, :ref => :by_name)
# => {:name=>"name"}
# => by_name
lazy_find_prop({:name => "name"}, :ref => :by_name)
# => {:name=>"name"}
# => by_name

lazy_find_orig({:name => "name"}, {:ref => :by_name})
# => (pry):7: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:name=>"name"}
# => by_name
lazy_find_prop({:name => "name"}, {:ref => :by_name})
# => (pry):7: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:name=>"name"}
# => by_name

# NOTE^: Same...we have to change the callers regardless

lazy_find_orig(:name => "name", :ref => :by_name)
# => (pry):4: warning: Passing the keyword argument as the last hash parameter is deprecated
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:name=>"name", :ref=>:by_name}
# => default_ref
lazy_find_prop(:name => "name", :ref => :by_name)
# => {:name=>"name"}
# => by_name

# NOTE^: This one is straight up broken on 2.7, but is also one that developers
#        will likely do because it feels natural.  Ladas noticed this and has an
#        explicit test for it to prevent it.
#        Proposal now allows this to work and doesn't produce wrong values, so
#        the test probably can be removed.

jrafanie · 2022-01-12T19:45:43Z

Updated example using RUBYOPT='-w' irb on Ruby 2.7

Interesting. I'm ok with holding off on this one. I feel like I want to see the changes needed in other repositories to see if a generalized solution like you're suggesting can be used to more surgically fix things in a way that makes sense while still fixing obviously wrong things like passing a hash as a second argument when we really wanted kwargs:

lazy_find_orig({:name => "name"}, {:ref => :by_name})
# => (pry):7: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:name=>"name"}
# => by_name
lazy_find_prop({:name => "name"}, {:ref => :by_name})
# => (pry):7: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
# => (pry):1: warning: The called method `lazy_find' is defined here
# => {:name=>"name"}
# => by_name

# NOTE^: Same...we have to change the callers regardless

Fryguy · 2022-01-19T16:33:01Z

@jrafanie it seems the guides/.rubocop_base.yml is no longer cached on github since it was deleted in Oct 2020. I've cherry-picked #96 back to v0.2.z, so can you rebase?

miq-bot · 2022-01-19T21:17:27Z

Checked commits jrafanie/inventory_refresh@ca9e487~...bdde38a with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
10 files checked, 19 offenses detected

spec/persister/local_db_finders_spec.rb

❗ - Line 163, Col 23 - Style/TrailingCommaInArguments - Avoid comma after the last parameter of a method call.
❗ - Line 293, Col 23 - Style/TrailingCommaInArguments - Avoid comma after the last parameter of a method call.

spec/persister/retention_strategies_spec.rb

❗ - Line 470, Col 116 - Layout/SpaceInsideHashLiteralBraces - Space inside } detected.
❗ - Line 470, Col 49 - Layout/SpaceInsideHashLiteralBraces - Space inside { detected.
❗ - Line 474, Col 116 - Layout/SpaceInsideHashLiteralBraces - Space inside } detected.
❗ - Line 474, Col 49 - Layout/SpaceInsideHashLiteralBraces - Space inside { detected.
❗ - Line 478, Col 116 - Layout/SpaceInsideHashLiteralBraces - Space inside } detected.

spec/persister/serializing_spec.rb

❗ - Line 116, Col 84 - Style/TrailingCommaInArguments - Avoid comma after the last parameter of a method call.
❗ - Line 120, Col 84 - Style/TrailingCommaInArguments - Avoid comma after the last parameter of a method call.

spec/save_inventory/acyclic_graph_of_inventory_collections_spec.rb

miq-bot · 2023-02-27T00:04:57Z

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

Fryguy · 2023-03-13T13:25:54Z

@jrafanie @agrare Is this PR still needed?

agrare · 2023-03-13T14:27:56Z

Ruby 3 compatibility was implemented by #112

jrafanie · 2023-03-13T14:30:15Z

Ruby 3 compatibility was implemented by #112

and #109

jrafanie added ruby3 wip labels Jan 7, 2022

agrare reviewed Jan 7, 2022

View reviewed changes

jrafanie mentioned this pull request Jan 10, 2022

Support ruby 3 ManageIQ/manageiq#21531

Merged

29 tasks

kbrock approved these changes Jan 11, 2022

View reviewed changes

jrafanie changed the title ~~lazy_find has hash and kwargs params: make both explicit for ruby 3~~ [WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 Jan 12, 2022

jrafanie added 4 commits January 19, 2022 16:14

lazy_find has hash and kwargs params: make both explicit for ruby 3

ca9e487

Unpack hash for kwargs with ruby 3.

7ac2211

lazy_find has hash and kwargs params: make both explicit for ruby 3

13be202

Explicitly pass keyword arguments to load for ruby 3 support

bdde38a

jrafanie force-pushed the v02z_ruby3_compatibility branch from b92e780 to bdde38a Compare January 19, 2022 21:15

This was referenced Aug 10, 2022

Add ruby3 compliant parameters for lazy_find #112

Merged

Handle two hash arguments to lazy_find #114

Merged

miq-bot added the stale label Feb 27, 2023

jrafanie closed this Mar 13, 2023

jrafanie deleted the v02z_ruby3_compatibility branch March 13, 2023 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 #97

[WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 #97

jrafanie commented Jan 7, 2022 •

edited

Loading

agrare commented Jan 7, 2022

agrare Jan 7, 2022

jrafanie Jan 7, 2022

Fryguy Jan 7, 2022 •

edited

Loading

agrare Jan 7, 2022 •

edited

Loading

Fryguy Jan 7, 2022

agrare Jan 7, 2022

agrare Jan 7, 2022

Fryguy Jan 7, 2022

kbrock Jan 11, 2022

jrafanie Jan 11, 2022 •

edited by Fryguy

Loading

kbrock left a comment

kbrock Jan 11, 2022

Fryguy commented Jan 11, 2022 •

edited

Loading

jrafanie commented Jan 11, 2022

agrare commented Jan 11, 2022

Fryguy commented Jan 12, 2022 •

edited

Loading

Fryguy commented Jan 12, 2022

Fryguy commented Jan 12, 2022 •

edited

Loading

jrafanie commented Jan 12, 2022

Fryguy commented Jan 19, 2022

miq-bot commented Jan 19, 2022

miq-bot commented Feb 27, 2023

Fryguy commented Mar 13, 2023

agrare commented Mar 13, 2023

jrafanie commented Mar 13, 2023

[WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 #97

[WIP] lazy_find has hash and kwargs params: make both explicit for ruby 3 #97

Conversation

jrafanie commented Jan 7, 2022 • edited Loading

agrare commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

agrare Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie Jan 11, 2022 • edited by Fryguy Loading

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Jan 11, 2022 • edited Loading

jrafanie commented Jan 11, 2022

agrare commented Jan 11, 2022

Fryguy commented Jan 12, 2022 • edited Loading

Fryguy commented Jan 12, 2022

Fryguy commented Jan 12, 2022 • edited Loading

jrafanie commented Jan 12, 2022

Fryguy commented Jan 19, 2022

miq-bot commented Jan 19, 2022

miq-bot commented Feb 27, 2023

Fryguy commented Mar 13, 2023

agrare commented Mar 13, 2023

jrafanie commented Mar 13, 2023

jrafanie commented Jan 7, 2022 •

edited

Loading

Fryguy Jan 7, 2022 •

edited

Loading

agrare Jan 7, 2022 •

edited

Loading

jrafanie Jan 11, 2022 •

edited by Fryguy

Loading

Fryguy commented Jan 11, 2022 •

edited

Loading

Fryguy commented Jan 12, 2022 •

edited

Loading

Fryguy commented Jan 12, 2022 •

edited

Loading