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

Proposal: alternative to Redis::BaseObject.expiration_filter #174

Merged
merged 10 commits into from May 1, 2015

Conversation

rossta
Copy link
Contributor

@rossta rossta commented Apr 30, 2015

PRs #172 and #173 highlight a weakness in the current implementation supporting expiration options on class macros, notably in Redis::BaseObject.expiration_filter:

def expiration_filter(*names)

This approach requires maintainers to register all appropriate methods and theirs aliases with expiration_filter. This is problematic for Redis::Objects which makes heavy use of alias methods in subclasses.

An alternative approach ideally obviates the need for a separate registry, makes it explicit that expiration is supported within the method definition itself, and handles alias methods seamlessly.

I propose replacing the current implementation with the following:

# Redis::BaseObject
def allow_expiration(&block)
  block.call.tap { set_expiration }
end

Subclasses would simply pass the method implementation as a block to allow_expiration:

# Redis::HashKey
def store(field, value)
  allow_expiration do
    redis.hset(key, field, marshal(value, options[:marshal_keys][field]))
  end
end
alias_method :[]=, :store

This PR depends on tests implemented in #172 and #173.

@nateware nateware merged commit c8e2a13 into nateware:master May 1, 2015
@nateware
Copy link
Owner

nateware commented May 1, 2015

Thanks Ross! Hopefully this fixes a few outstanding bugs as well.

@rossta
Copy link
Contributor Author

rossta commented May 1, 2015

@nateware Thanks for the quick merge and version bump!

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.

None yet

2 participants