sorentwo / readthis

:newspaper: Pooled active support compliant caching with redis
MIT License
504 stars 40 forks source link

Fixed merged_options method #28

Closed tobinibot closed 8 years ago

tobinibot commented 8 years ago

This PR addresses issue #27.

In the merged_options method, the order of the arguments was wrong; we want to merge the specific options into the default options, and not the other way around. The original code was overwriting the specific options hash with values from the default options hash.

# original method
def merged_options(options)
    (options || {}).merge!(@options)
end

# fixed method
def merged_options(options)
    @options.merge(options || {})
end

There also doesn't seem to be any need to permanently update the specific options hash (returning a properly merged hash is good enough), so I didn't worry about copying that functionality over to the fixed method.

The reason the test suite didn't catch this is that the default expires_in period is 1 second and in the test for custom expiration, it also uses 1 second. I added a test which mirrors my setup (a high default and a short specific period), and the test suite passes successfully again after the code change.

sorentwo commented 8 years ago

Thanks for tracking this down! Will you please just change the variable name in the spec and add a CHANGELOG entry to give yourself credit? I'll merge afterwards.

sorentwo commented 8 years ago

@tobinibot I didn't realize you had pushed up some fixes. One last thing, will you please squash these into a single commit?

tobinibot commented 8 years ago

Sure, haven't ever done that before. Let me go figure it out.

sorentwo commented 8 years ago

@tobinibot You'll want to use git rebase -i master to squash up the various commits.

http://www.git-scm.com/book/en/v2/Git-Tools-Rewriting-History

tobinibot commented 8 years ago

Commits squashed

sorentwo commented 8 years ago

Thanks @tobinibot