Closed jrafanie closed 1 year ago
agrare
commented
2 years ago 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.
Fryguy
commented
2 years ago EDIT: Ignore these examples, because they are wrong...see https://github.com/ManageIQ/inventory_refresh/pull/97#issuecomment-1011156232
jrafanie
commented
2 years ago 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_hashthen I think we can keep the same original signature and no warnings. We may also want to add a few more
ArgumentErrorchecks that the user can't pass both nor neither ofmanager_uuidandmanager_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
commented
2 years ago 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
commented
2 years ago @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
commented
2 years ago 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
commented
2 years ago 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.Updated example using
RUBYOPT='-w' irbon 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
commented
2 years ago @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
commented
2 years ago spec/persister/local_db_finders_spec.rb
spec/persister/retention_strategies_spec.rb
spec/persister/serializing_spec.rb
spec/save_inventory/acyclic_graph_of_inventory_collections_spec.rb
miq-bot
commented
1 year ago 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
commented
1 year ago @jrafanie @agrare Is this PR still needed?
agrare
commented
1 year ago Ruby 3 compatibility was implemented by https://github.com/ManageIQ/inventory_refresh/pull/112
jrafanie
commented
1 year ago Ruby 3 compatibility was implemented by #112
and #109
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.