Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add hocon support #6

Merged
merged 17 commits into from Mar 8, 2021
Merged

Add hocon support #6

merged 17 commits into from Mar 8, 2021

Conversation

CDFN
Copy link
Contributor

@CDFN CDFN commented Dec 13, 2020 •
edited

Partially resolves #2. Still we're missing comment support. Also there's weird bug -

public interface Config {
  @DefaultString("someString")
  String testString();
}

after writing to file results in

{
    # hardcoded value
    "testString" : "someString"
}

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.

@CDFN CDFN changed the title Add basic hocon support Add hocon support Dec 13, 2020
A248
A248 reviewed Dec 13, 2020
View reviewed changes
Copy link
Owner

@A248 A248 left a 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
@A248
Copy link
Owner

A248 commented Dec 14, 2020

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

@A248
Copy link
Owner

A248 commented Dec 14, 2020

With that in mind, I understand how to add comment support:

  1. Convert DazzleConf's Map<String, Object> to lightbend-config's Map<String, ConfigValue>. ConfigValueFactory will assist in this.
  2. When converting the map values (Object -> ConfigValue), add comments with ConfigValue#withOrigin and ConfigOrigin#withComments
  3. Render the complete ConfigValue which is derived from the Map<String, ConfigValue>.

@CDFN
Copy link
Contributor Author

CDFN commented Dec 14, 2020

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 😃

@A248
Copy link
Owner

A248 commented Dec 17, 2020

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.

  • Origin comments being enabled (setOriginComments) is what causes "hardcoded value" to show up, so origin comments should be disabled by default.
  • setJson would be best set to false by default. It somewhat defeats the point of using hocon over json otherwise.

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

@A248 A248 added this to the 1.2.0 milestone Dec 18, 2020
A248
A248 reviewed Jan 19, 2021
View reviewed changes
Copy link
Owner

@A248 A248 left a 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
@A248 A248 marked this pull request as ready for review March 8, 2021 19:44
A248
A248 approved these changes Mar 8, 2021
View reviewed changes
Copy link
Owner

@A248 A248 left a 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!

@A248 A248 merged commit 59fd6af into A248:master Mar 8, 2021
@CDFN
Copy link
Contributor Author

CDFN commented Mar 8, 2021

Thanks for finishing it. Glad I could help even a little 🙂👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@A248 A248 A248 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.2.0
Development

Successfully merging this pull request may close these issues.

More configuration format support
2 participants
@CDFN @A248