-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add hocon support #6
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regards to exception handling, AbstractConfigurationFactory#loadMapFromReader can be loosened to throw InvalidConfigException. Then you'll be able to better map HOCON's exceptions.
One more thing, have you tested nested configuration sections? I'm not sure ConfigFactory#parseMap is the right call, but I believe it is ConfigValueFactory#fromMap. ConfigValueFactory says its 'fromAnyRef' (which fromMap calls) is the inverse operation of ConfigObject#unwrapped.
On the decision to expose ConfigParseOptions, could you elaborate on what this would be used for? Some of the options in ConfigParseOptions do not seem pertinent to DazzleConf, though I'll admit I'm a little hazy on their purpose, so I may need to look closer into this.
I'll also be available to provide advice on implementing comment support. First, let's iron out these details.
hocon/src/main/java/space/arim/dazzleconf/ext/hocon/HoconOptions.java
Outdated
Show resolved
Hide resolved
hocon/src/main/java/space/arim/dazzleconf/ext/hocon/HoconConfigurationFactory.java
Outdated
Show resolved
Hide resolved
While looking into comment support, I found out where "hardcoded value" comes from: https://github.com/lightbend/config/blob/ea45ea3767a201933eeeb9c3f0f13eacc9b51f07/config/src/main/java/com/typesafe/config/impl/ConfigImpl.java#L156 |
With that in mind, I understand how to add comment support:
|
Thank you for support with implementing it. I haven't been testing nested sections yet. I decided to expose ParseOptions to make parsing config more flexible. Should I wrap underlying options in ParseOptions into HoconOptions so there's no need to use hocon lib's objects? Also thanks for explaining how should I implement comments. I'll try my best 😃 |
On further thought, I think you are right to expose the formatting library's own options in full, as this is what is done by the other format extensions (Gson and SnakeYaml). However, I think ConfigRenderOptions should have saner defaults.
Let me know if you disagree with my reasoning on these. I'm not an everyday HOCON user so you likely know more than I do. Also, feel free to add yourself to the section in the parent pom.xml |
hocon/src/main/java/space/arim/dazzleconf/ext/hocon/HoconOptions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For new configuration formats, it would be better to encapsulate the identity of the ConfigurationFactory implementations, using static create
methods rather than constructors.
The tomlj-support branch has an example of this:
https://github.com/A248/DazzleConf/blob/toml-support/tomlj/src/main/java/space/arim/dazzleconf/ext/tomlj/TomljConfigurationFactory.java
hocon/src/main/java/space/arim/dazzleconf/ext/hocon/HoconOptions.java
Outdated
Show resolved
Hide resolved
hocon/src/main/java/space/arim/dazzleconf/ext/hocon/HoconOptions.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took care of the rest. Thanks for contributing!
Thanks for finishing it. Glad I could help even a little 🙂👍 |
Partially resolves #2. Still we're missing comment support. Also there's weird bug -
after writing to file results in
even though the comment was never created by me. Help with troubleshooting would be appreciated :)
Addonationaly, better exception handling. Currently in HoconConfigurationFactory#loadMapFromReader only ConfigException.Parse is handled, otherwise IOException is thrown.