Skip to content

Conversation

kastoestoramadus
Copy link
Contributor

@kastoestoramadus kastoestoramadus commented Apr 23, 2025

Added:

  • keep origin order of entries,
  • switch two/four spaces,
  • assing sign switch : or =

Why? lack of good formatter that can be used with git hooks. This lib allows ignoring resolving the references.

@ekrich
Copy link
Owner

ekrich commented Apr 23, 2025

@kastoestoramadus Thanks for this pull request. I will work with you to try and understand this improvement and get it to the finish line. You will need to run scripts/scalafmt to get pass the format check. Note that this project is cross built for 2.12,1.13,3.x so some new Scala features/formats may not work.

  1. I saw that you created a pull request on the Java based repo but I don't think they are adding new features. Are you using this repo or thinking of switching?
  2. I would like to hear more about the use case so I am hoping you can write a thorough example etc. for me and for future users. I am not a very experienced user of this library.

}

@Test
def keepOriginOrderOfEntries(): Unit = {
Copy link
Contributor Author

@kastoestoramadus kastoestoramadus Apr 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekrich
Before, entries always were sorted in alphabetical order.

with

  • ConfigFactory.parseFile
  • render
  • and save to file
    a formatter is ready to go.

@@ -79,7 +81,7 @@ object SimpleConfigObject {
}

@SerialVersionUID(1L)
final private class RenderComparator
private class RenderComparator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where this guarantee is used.
What rule does my new comparator have to obey?

@ekrich
Copy link
Owner

ekrich commented Apr 25, 2025

@kastoestoramadus I was wondering if you could answer the questions I asked before. I do this part time so I don't have much time to devote to major changes.

  1. You originally had a PR to lightbend config. Are you using this library now instead?
  2. I understand about rendering which is a big subject in the original library. What exactly is your use case for this change?

Note: In the history of this library - render was designed for debugging - not for a roundtrip. For example this PR was designed for an advanced render but is a huge change - #319

This is a very stable library. I would like to improve it but I have to be very careful to not break binary compatibility. A couple of your changes potentially break compatibility. I have mima setup but need to fix in the build.

To check run: sconfigJVM/mimaReportBinaryIssues

@kastoestoramadus
Copy link
Contributor Author

kastoestoramadus commented Apr 27, 2025

hi,
ad 1) We were using the lightbend lib version heavily for parsing and merging multiple config files. I've tested yours and it works fine in my internal network and is open for development to fix formatting blockers.
ad 2) I've found no formatter for HOCON that can skip resolving the references. As for now I can use this library as it always reorder the entries in alphabetical order.
IMO: it's a shame that no proper formatter exists for HOCON. The only formatter working properly I've found is bounded with Intelij Idea API.
I aim at integration with git hooks.

I'll check this sconfigJVM/mimaReportBinaryIssues, thanks. Old tests from sbt test just pass for now without touching them. All defaults are set to be compatible with the old settings.

Do you share this mima somewhere?

@kastoestoramadus
Copy link
Contributor Author

kastoestoramadus commented Apr 28, 2025

If render was only for debugging then my changes shouldn't be risky to add as a new production feature to it as long, as it get parsed back to the same model ;).
If you prefer more Java code style, let me know.

@ekrich
Copy link
Owner

ekrich commented Jun 25, 2025

Closed via #454

@ekrich ekrich closed this Jun 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants