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

simpler convert_to_hash function #176

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

robmbrooks
Copy link

if real_size is zero, ie

:key => 'foo'
:value => 'bar'

convert_to_hash creates 'foo '=> {} rather than 'foo' => 'bar'

@pierresouchay
Copy link
Member

@robmbrooks Can you rebase to let Travis tests pass?

@pierresouchay pierresouchay added question needs-rebase Needs rebase the PR labels Feb 25, 2019
@robmbrooks robmbrooks force-pushed the master branch 3 times, most recently from cf34c20 to bb05fa1 Compare February 26, 2019 04:44
@robmbrooks
Copy link
Author

@pierresouchay let me know if this works for you!

@pierresouchay
Copy link
Member

@robmbrooks Can you please explain a bit the context?

When is the behavior problematic?

@robmbrooks
Copy link
Author

@pierresouchay

If you create a key of foo in consul with a value of bar, the convert_to_hash function returns foo => {}.

The issue being that real_size = 0, so this code never gets to assign item[:value]

        real_size = split_up.size - 1
        (0..real_size).each do |i|
          if i.zero?
            temp = {}
            sub_hash[split_up[i]] = temp
            next
          end
          if i == real_size
            temp[split_up[i]] = item[:value]
          else

@robmbrooks
Copy link
Author

robmbrooks commented Feb 27, 2019

@pierresouchay

I've added an rspec test that tests for the issue within the pull request

@pierresouchay
Copy link
Member

@robmbrooks Thank you for the Unit test, this is now self-explanatory :-)

@pierresouchay pierresouchay merged commit ef57278 into WeAreFarmGeek:master Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Needs rebase the PR question
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants