rubyconfig/config

Config::Options: to_hash and to_h have different behaviour

thomas-mcdonald opened this issue · 11 comments

thomas-mcdonald commented

I would expect to_h and to_hash to have the same behaviour, however looks like this gem only overrides the definition of to_hash and does not alias that definition with to_h.

>> Settings.elasticsearch.to_h
=> {:host=>"localhost", :log=>true, :test=>#<Config::Options test=true>}
>> Settings.elasticsearch.to_hash
=> {:host=>"localhost", :log=>true, :test=>{:test=>true}}

Happy to file a patch with the alias - just wanted to check this wasn't the desired behaviour.

pkuczynski commented

@rdubya what you think?

rdubya commented

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.

pkuczynski commented

@thomas-mcdonald would you still be willing to provide PR for this issue?

pkuczynski commented

@pyromaniac what you think about this one?

pkuczynski commented

@thomas-mcdonald are you still interested in fixing this?

Fryguy commented

@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

pkuczynski commented

Damn. I will revert now and release the next minor.

@Fryguy how should we approach #217 in another way? Or maybe we should not at all?

Fryguy commented

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.

pkuczynski commented

Of course. SemVer is important. Should we bump it to 3.x or rather do 2.3 though? Open for feedback from others... @cjlarose?

cjlarose commented

3.0.0 seems appropriate to me. I agree that making to_h match the behavior of to_hash is a good thing to do.

  • 👍1
pkuczynski commented

Created #297 to re-introduce this change in 3.0.0

  • 👍1