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

Adds cached_client method in BaseConnection #371

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Conversation

dmccown
Copy link
Contributor

@dmccown dmccown commented Oct 22, 2018

Adds a helper method in base connection so you no longer have to
duplicate caching logic in any plugins you may create. Example:

def demo_client
  cache_client(:api_call, :demo_connection) do
    DemoClient.new(options)
  end
end

This will automatically check if the cache is enabled and return the the
cached object if it exists. If it doesn't exist then the cache will be
hydrated.

Signed-off-by: David McCown dmccown@chef.io

Adds a helper method in base connection so you no longer have to
duplicate caching logic in any plugins you may create. Example:

```Ruby
def demo_client
  cache_client(:api_call, :demo_connection) do
    DemoClient.new(options)
  end
end
```

This will automatically check if the cache is enabled and return the the
cached object if it exists. If it doesn't exist then the cache will be
hydrated.

Signed-off-by: David McCown <dmccown@chef.io>
def cached_client(type, key)
return yield unless cache_enabled?(type)

@cache[type][key] ||= yield

Choose a reason for hiding this comment

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

@cache.dig(type, key) ||= yield?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if you could do assignment when using dig.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't work. You aren't allowed to do assignment from the return of dig.

train/lib/train/plugins/base_connection.rb:56: syntax error, unexpected tOP_ASGN,
expecting keyword_end (SyntaxError)
      @cache.dig(type, key) ||= yield

Choose a reason for hiding this comment

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

Yeah, you're right. I wasn't thinking about the assignment. That's a shame--it should work that way!

conn = DemoConnection.new
conn.enable_cache(:api_call)

client1 = conn.demo_client
Copy link

@TrevorBramble TrevorBramble Oct 22, 2018

Choose a reason for hiding this comment

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

It wouldn't be useful in the other tests, because they don't look anything like real world code, but this one made me wonder if we could just return self from enable_cache() and disable_cache(). I quick search shows that the implicit return of a boolean is never treated as significant.

def enable_cache(type)
  fail Train::UnknownCacheType, "#{type} is not a valid cache type" unless @cache_enabled.keys.include?(type.to_sym)

  @cache_enabled[type.to_sym] = true

  self
end
it 'returns a new connection when cache enabled and not hydrated' do
  conn = DemoConnection.new.enable_cache(:api_call)

  client1 = conn.demo_client
  client1.must_be_instance_of DemoConnection::DemoClient
end

# or even...

it 'returns a new connection when cache enabled and not hydrated' do
  client1 = DemoConnection.new.enable_cache(:api_call).demo_client

  client1.must_be_instance_of DemoConnection::DemoClient
end

Copy link
Contributor

@jquick jquick left a comment

Choose a reason for hiding this comment

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

Thanks @dmccown

@jquick jquick merged commit f40600b into master Nov 1, 2018
@zenspider zenspider deleted the dmccown/client-caching branch February 1, 2020 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants