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

Inherit key coercions #256

Closed
wants to merge 5 commits into from
Closed

Conversation

Erol
Copy link
Contributor

@Erol Erol commented Dec 27, 2014

This fixes #255.

@dblock
Copy link
Member

dblock commented Dec 27, 2014

Any better way to do this than via an instance_variable_set? Maybe expose a property?

Looks good otherwise, needs a CHANGELOG entry please.

@Erol
Copy link
Contributor Author

Erol commented Dec 27, 2014

How about this? I created a protected key_coercions= property setter to setup up inherited coercions.

On another note, I noticed that most properties were defined as:

def strict_value_coercions
  @strict_value_coercions || {}
end

And then conditionally initialized and used:

if options[:strict]
  (@strict_value_coercions ||= {})[from] = into
else
  ...
end

How about if we redefine them as:

def strict_value_coercions
  @strict_value_coercions ||= {}
end

And use them immediately, without worrying about initialization? It looks cleaner:

if options[:strict]
  strict_value_coercions[from] = into
else
  ...
end

Your thoughts?

@dblock
Copy link
Member

dblock commented Dec 28, 2014

This is perfect, merged via a493872.

Your proposal seems sensible, feel free to PR further.

@dblock dblock closed this Dec 28, 2014
@Erol Erol deleted the inherit-key-coercions branch March 16, 2015 06:05
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.

coerce_key doesn't work in subclasses
2 participants