-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
Config::Options: to_hash and to_h have different behaviour #217
Comments
@rdubya what you think? |
I don't have much experience with implementing these types of methods, but based on this article (https://zverok.github.io/blog/2016-01-18-implicit-vs-expicit.html) it seems like to_h should probably be the one that people should be using, so I'm good with aliasing it. |
@thomas-mcdonald would you still be willing to provide PR for this issue? |
@pyromaniac what you think about this one? |
@thomas-mcdonald are you still interested in fixing this? |
@pkuczynski The PR for this seems to have broken backward compatibility, and probably should warrant a major release according to SemVer. Not sure how to proceed, but we may want to revert this for the 2.2 line. For example, we had this code (roughly) which is now broken... foo:
bar: true Settings.to_h.select { |k, v| v.bar }
# Before
# => {:foo=>#<Config::Options bar=true>}
# After
# undefined method `bar' for {:bar=>true}:Hash (NoMethodError) That is, the code was working with the existing .to_h contract of returning a Hash(Symbol, Config::Options), which allowed dot-access on the value. Now the dot-access is lost. See also ManageIQ/manageiq#20882 |
I think we can keep it and just cut a 3.0.0 as per SemVer. FWIW, I personally like this PR as it's much more intuitive moving forward. I think the fact that we essentially inherited .to_h from OpenStruct was more of an accident, but I also think following SemVer and not breaking things is more important. |
Of course. SemVer is important. Should we bump it to 3.x or rather do 2.3 though? Open for feedback from others... @cjlarose? |
3.0.0 seems appropriate to me. I agree that making |
Created #297 to re-introduce this change in 3.0.0 |
I would expect
to_h
andto_hash
to have the same behaviour, however looks like this gem only overrides the definition ofto_hash
and does not alias that definition withto_h
.Happy to file a patch with the alias - just wanted to check this wasn't the desired behaviour.
The text was updated successfully, but these errors were encountered: