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

No longer assume that a value is an array #16924

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

syncrou
Copy link
Contributor

@syncrou syncrou commented Jan 31, 2018

We found a situation where an ||= was failing because the result was nil as opposed to an array.
Adding a .try to the lookup allows us to pass nil in these specific cases.

https://bugzilla.redhat.com/show_bug.cgi?id=1540326

@syncrou
Copy link
Contributor Author

syncrou commented Jan 31, 2018

@miq-bot add_label bug, services

@miq-bot assign @lfu

cc @gmcculloug

@syncrou
Copy link
Contributor Author

syncrou commented Jan 31, 2018

@miq-bot add_label blocker

@syncrou
Copy link
Contributor Author

syncrou commented Jan 31, 2018

@miq-bot add_label 'gaprindashvili/yes'

@syncrou
Copy link
Contributor Author

syncrou commented Jan 31, 2018

@AlonaKaplan - Pinging you on this. We found a case where @values[fn] was nil, causing the .first to raise an Exception. Just an FYI. If you have any additional feedback please let me know.

@miq-bot
Copy link
Member

miq-bot commented Jan 31, 2018

@syncrou Cannot apply the following label because they are not recognized: 'gaprindashvili/yes'

@syncrou
Copy link
Contributor Author

syncrou commented Jan 31, 2018

@miq-bot add_label gaprindashvili/yes

@djberg96
Copy link
Contributor

I prefer vlan ||= Array(@values[fn]).first because it also handles the possibility that @values[fn] ever returns a single object, or a value that responds to .first but isn't an array element. Basically, cast it first, then access it.

Not a huge deal, but I vaguely recall smacking into this with the Azure refresh parser at some point.

We found a situation where an ||= was failing because the result was nil as opposed to an array.
Adding a .try to the lookup allows us to pass nil in these specific cases.

https://bugzilla.redhat.com/show_bug.cgi?id=1540326
@syncrou syncrou force-pushed the add_try_to_array_lookup branch from 2cce2b9 to 698a346 Compare February 1, 2018 14:34
@miq-bot
Copy link
Member

miq-bot commented Feb 1, 2018

Checked commit syncrou@698a346 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Rubocop - Linter::Rubocop STDERR:
An error occurred while Rails/Presence cop was inspecting /tmp/d20180201-10518-9l5xdo/app/models/miq_provision_virt_workflow.rb:670:98.
To see the complete backtrace run rubocop -d.

1 error occurred:
An error occurred while Rails/Presence cop was inspecting /tmp/d20180201-10518-9l5xdo/app/models/miq_provision_virt_workflow.rb:670:98.
Errors are usually caused by RuboCop bugs.
Please, report your problems to RuboCop's issue tracker.
Mention the following information in the issue report:
0.52.0 (using Parser 2.4.0.2, running on ruby 2.3.3 x86_64-linux)

@gmcculloug
Copy link
Member

@lfu Please review.

Note: @syncrou and I discussed refactoring this method for easier testing in a separate PR.

@lfu
Copy link
Member

lfu commented Feb 1, 2018

@syncrou LGTM 👍 Can you also add test cases?

@syncrou
Copy link
Contributor Author

syncrou commented Feb 1, 2018

@lfu - I have the beginnings of a refactor with tests worked up. I was planning on a followup PR to handle them so we can close this blocker.

@lfu
Copy link
Member

lfu commented Feb 1, 2018

@syncrou That works for me.
This PR can be merged.

@gmcculloug gmcculloug merged commit 27b975a into ManageIQ:master Feb 1, 2018
@gmcculloug gmcculloug added this to the Sprint 79 Ending Feb 12, 2018 milestone Feb 1, 2018
simaishi pushed a commit that referenced this pull request Feb 1, 2018
@simaishi
Copy link
Contributor

simaishi commented Feb 1, 2018

Gaprindashvili backport details:

$ git log -1
commit b1c1ae056a72a9ea9a639820537c381197fd71cb
Author: Greg McCullough <[email protected]>
Date:   Thu Feb 1 16:18:11 2018 -0500

    Merge pull request #16924 from syncrou/add_try_to_array_lookup
    
    No longer assume that a value is an array
    (cherry picked from commit 27b975a6234a38dca0236149d1daaa3ee714f88e)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1541171

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.

6 participants