puppetlabs / ruby-hocon

A Ruby port of the Typesafe Config library.
Apache License 2.0
34 stars 30 forks source link

Convert symbols to strings #60

Closed kjw1 closed 9 years ago

kjw1 commented 9 years ago

Right now, if you supply a hash with symbols in it to ConfigValueFactory.parse:

irb(main):014:0> b = Hocon::ConfigValueFactory.from_map( { a: :b } )
Hocon::ConfigError::ConfigBugOrBrokenError: bug in method caller: not valid to create ConfigObject from map with non-String key: a
    from /Users/kwong/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/hocon-0.9.2/lib/hocon/impl/config_impl.rb:119:in `block in from_any_ref_mode'
    from /Users/kwong/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/hocon-0.9.2/lib/hocon/impl/config_impl.rb:117:in `each'
    from /Users/kwong/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/hocon-0.9.2/lib/hocon/impl/config_impl.rb:117:in `from_any_ref_mode'
    from /Users/kwong/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/hocon-0.9.2/lib/hocon/impl/config_impl.rb:72:in `from_any_ref'
    from /Users/kwong/.rbenv/versions/2.1.2/lib/ruby/gems/2.1.0/gems/hocon-0.9.2/lib/hocon/config_value_factory.rb:72:in `from_map'
    from (irb):14
    from /Users/kwong/.rbenv/versions/2.1.2/bin/irb:11:in `<main>'

It gives an error. Can the symbols be automatically converted to Strings when converting to a HOCON document? Is there a downside of doing so?

cprice404 commented 9 years ago

Seems pretty reasonable to me, what do you think @fpringvaldsen ?

MSLilah commented 9 years ago

@kjw1 Thanks for bringing this to our attention! @kjw1 @cprice404 I 100% agree that we should support hashes with symbolic keys, and I can see two ways of going about this.

I think we could definitely go with @kjw1's suggested approach above. It has the advantage of only minor code divergence from the upstream (we'd just have to convert all the symbols to strings before parsing a hash) and being really easy to implement.

In this case though, if someone tried to insert a hash with symbolic keys into a configuration, those keys would be inserted as strings, rather than as non-string keys, which the HOCON spec and parser support. So if you parsed and inserted { a: 1 }, you'd get a result of "a" : 1 in your configuration.

I think my preferred approach would be to actually support the parsing of symbols as keys when parsing a ruby hash, and the insertion of those keys into configurations as non-string keys. In this case, if you parsed and inserted { a: 1 }, you'd get a result of a : 1 in your configuration. I think this behavior is more intuitive, but it has the drawbacks of being more difficult to implement, and will also cause a larger amount of code divergence from the upstream library.

cprice404 commented 9 years ago

@fpringvaldsen that sounds reasonable... can you elaborate a bit more on what the advantages are of non-string keys over string keys?

cprice404 commented 9 years ago

oops, didn't mean to close

MSLilah commented 9 years ago

@cprice404 In terms of the HOCON language itself I don't know if there's any particular advantage to using non-string keys aside from the fact that it allows you to have a key be a multi-element path (you could have a.b.c in your configuration file, which would map to a { b { c : [value] } }), although I believe the HOCON spec recommends against doing this.

I think the real advantage is that non-string keys can be used in code (at least in clojure) as keywords rather than strings, which is how we've been using them in trapperkeeper and projects that use it. Also, most of our trapperkeeper projects require non-string keys in their configuration files.

cprice404 commented 9 years ago

@fpringvaldsen ok, thanks, that helps.

Is there a data type that you can put into the input map that would get translated to a normal HOCON key?

Basically I think if we can implement this as a pre-processing step, where we do some ruby-specific stuff to the input map before handing it off to the more formal HOCON API, then we probably should. But if we'd have to scatter knowledge about Ruby keywords throughout other parts of the HOCON code, I'd be a little less comfortable with it and would probably opt for just translating them to Strings.

MSLilah commented 9 years ago

@cprice404 As of now I don't think it's possible to translate a key in a hash to a normal HOCON key, since HOCON can only parse hashes that have String keys (this is true of ruby-hocon and the upstream typesafe library). If we do this as a pre-processing step, we'll have to just convert keys to strings before handing a hash off to the ConfigValueFactory, as doing otherwise would definitely require changing other parts of the HOCON code, and modifying parsing to have knowledge of ruby.

cprice404 commented 9 years ago

So in the upstream library it'd be impossible to pass in a Java map and have it treat the keys as anything other than Strings? If that's the case then it definitely seems consistent for us to just pre-parse the ruby map and convert the keywords to strings.

cprice404 commented 9 years ago

I believe this was fixed by #62 .