to_h returns a hash with symbol keys

[faloi]

Although this is a breaking change, this is how OpenStruct behaves and how I expected your solution to work.

Do you have any special reason for preserving the original keys?

[aetherknight]

@Kris-LIBIS wrote the spec in question, although I suspect it is an oversight. I generally prefer defaulting to mimicking OpenStruct. @Kris-LIBIS any thoughts?

[Kris-LIBIS]

It is intentional. It asures that a roundtrip 'read file into OpenStruct and save back to file' will not alter untouched data. Changing the keys can - and will in our case - break other applications that share the information in the configuration file. If you change it, please make it configurable as otherwise it would bring down our application. I cannot change the 3rd party's application behaviour, so it is important to keep this behaviour.

[aetherknight]

@Kris-LIBIS Do you commonly have a case where a YAML file mixes string and symbol keys, or other kinds of keys? Basically, asking how useful an option to preserve the original keys would be, something like:

ros = RecursiveOpenStruct.new({...}, recurse_over_arrays: true, preserve_original_keys: true)
...
ros.to_h

vs doing something like:

ros = RecursiveOpenStruct.new({...}, recurse_over_arrays: true)
...
ros.to_h.reduce({}) { |memo, (k,v)| memo[k.to_s] = v ; memo }

[Kris-LIBIS]

I currently have one case where mixed key types are used. Unfortunately that's enough ;)

To be honest, I believe the mixed key type behaviour of the application should be considered a bug and the concept of server and client exchanging configuration info via a YAML file is bad design. But I do not expect that will be fixed any time soon.

In any other cases I can think of right now, there is only one consistent YAML writer and converting at output would not be an issue. It is fairly common though to want to force keys as symbols/strings that I would at least suggest to make it a method on RecursiveOpenStruct. But I think the code above would not convert {foo: [ { bar: 1 } ] } correctly.

[Kris-LIBIS]

This is not moving and I'm afraid I'm part of the problem. Maybe I should clarify things:

I currently do rely on the indifferent access behaviour, but I do support the fact that ROS should behave as much as possible like an OpenStruct. I'm strongly in favor of making ROS converting keys to symbols on input again as long as there is still an easy way to make it work as it does now. That could mean introducing an extra configuration parameter or defining a module that could be included to change the behaviour.

Unfortunately changing the default behaviour would make it a breaking change now, but as long as users of the gem have an easy way to revert to the I.A. behaviour, that may be acceptable? I do agree that making I.A. the default behaviour was a mistake and it's mine.

[aetherknight]

Do you think there might be much value in providing a minor release where the existing behavior is the default but we add an option to enable the strict behavior? The next major release would then change the default behavior and make indifferent access optional. This sounds like an extra set of logistical hoops to deal with compared to just making a new major version that makes the indifferent access optional though.

Also, if it seems that you are waiting for feedback or a decision from me and I am not responding within a few days or a week, feel free to comment on the bug or PR to get my attention.

On Thu, Oct 29, 2015 at 2:09 AM, Kris Dekeyser notifications@github.com wrote:

This is not moving and I'm afraid I'm part of the problem. Maybe I should clarify things:

I currently do rely on the indifferent access behaviour, but I do support the fact that ROS should behave as much as possible like an OpenStruct. I'm strongly in favor of making ROS converting keys to symbols on input again as long as there is still an easy way to make it work as it does now. That could mean introducing an extra configuration parameter or defining a module that could be included to change the behaviour.

Unfortunately changing the default behaviour would make it a breaking change now, but as long as users of the gem have an easy way to revert to the I.A. behaviour, that may be acceptable? I do agree that making I.A. the default behaviour was a mistake and it's mine.

— Reply to this email directly or view it on GitHub https://github.com/aetherknight/recursive-open-struct/pull/34#issuecomment-152120852 .

[faloi]

My problem was simpler than I thought, so I ended up using ActiveSupport's HashWithIndiferentAccess.

Now, I understand this gem solves a very specific need, so perhaps you should clarify that on the README. Otherwise, the approach being discussed here seems more "correct".

[Kris-LIBIS]

Maybe the best way to deal with this is to state in the README how ROS is currently behaving and that it will soon be changing. Making I.A. optional and strict behaviour default again would probably justify a version bump to 0.7, IMHO.

[aetherknight]

Closing this PR in favor of #40. @Kris-LIBIS @faloi could you review that PR and let me know if you see any problems? As soon as I get a thumbs up, I will merge it in to master, and I will look into getting #39 merge din.