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

Follow up on #54 "Fog google upgrade (to 1.3.3)" code cleanup #59

Merged
merged 1 commit into from
Jun 4, 2018

Conversation

tumido
Copy link
Member

@tumido tumido commented Jun 1, 2018

Let's follow up on #54 and incorporate some changes @agrare suggested.

Make the code neater and faster again!

@@ -180,7 +180,7 @@ def get_cloud_networks

def subnetworks
unless @subnetworks
@subnetworks = @connection.list_aggregated_subnetworks.to_h[:items].flat_map { |_, v| v[:subnetworks] }
@subnetworks = @connection.list_aggregated_subnetworks.to_h[:items].values.flat_map { |v| v[:subnetworks] }
Copy link
Member Author

Choose a reason for hiding this comment

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

@agrare, I couldn't do here what you've suggested. The :subnetworks is not an method but a key in Hash so we can't be calling it like:

@connection.list_aggregated_subnetworks.to_h[:items].values.flat_map(&:subnetworks)

I've tried also this:

@connection.list_aggregated_subnetworks.to_h[:items].values.pluck(:subnetworks).flatten

Despite it's being a bit better readable, it performs about twice slower than the flat_map version (due to the flatten). So I've left the flat_map here.

Copy link
Member

Choose a reason for hiding this comment

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

👍 it was the .values instead of { |_, v| that I most cared about

@miq-bot
Copy link
Member

miq-bot commented Jun 1, 2018

Checked commit tumido@524bc14 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

LGTM

@agrare agrare self-assigned this Jun 4, 2018
@agrare agrare merged commit 524bc14 into ManageIQ:master Jun 4, 2018
agrare added a commit that referenced this pull request Jun 4, 2018
Follow up on #54 "Fog google upgrade (to 1.3.3)" code cleanup
@tumido tumido deleted the fog_google_upgrade branch June 26, 2018 14:27
@simaishi
Copy link
Contributor

Fiine backport (to manageiq repo) details:

$ git log -1
commit 9522bb581db76771dcd8922944371a26919bc252
Author: Adam Grare <[email protected]>
Date:   Mon Jun 4 09:59:41 2018 -0400

    Merge pull request #59 from tumido/fog_google_upgrade
    
    Follow up on #54 "Fog google upgrade (to 1.3.3)" code cleanup
    (cherry picked from commit 466bbd579bfb5263779056f58a92c1785ad91479)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants