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.

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

Use JSON5 (JSON for humans) for parsing configurations. #334

Closed
garronej opened this issue Dec 20, 2023 · 7 comments
Closed

Use JSON5 (JSON for humans) for parsing configurations. #334

garronej opened this issue Dec 20, 2023 · 7 comments

Comments

@garronej
Copy link
Contributor

garronej commented Dec 20, 2023

Hello,

I suggest switching from regular JSON to JSON5 for parsing the catalog and region configuration.
It's much better suited for the usecase.
It would be great if a trailing coma wouldn't prevent the API to start.
Also it would be very nice to be able to comment out part of the configuration if needed (Regular JSON does not support comments).

I use implemented this approach for the onyxia-web configuration already as explained here: https://youtu.be/NrVuVXsbloA?si=Fnu7sEwLuYl-t1uA&t=796

@garronej garronej assigned olevitt and fcomte and unassigned olevitt and fcomte Dec 20, 2023
@garronej
Copy link
Contributor Author

Here is a JSON5 Java implementation: https://github.com/marhali/json5-java?tab=readme-ov-file

@johnksv
Copy link
Contributor

johnksv commented Jan 9, 2024

This could be a nice feature. I belive the built in jackson json parser could be tuned to match this behaviour: https://github.com/FasterXML/jackson-core/wiki/JsonReadFeatures
https://docs.spring.io/spring-boot/docs/3.1.4/reference/htmlsingle/#howto.spring-mvc.customize-jackson-objectmapper

@johnksv
Copy link
Contributor

johnksv commented Jan 9, 2024

Saw that a custom object mapper was used. I took the liberty to provide a PR

@garronej
Copy link
Contributor Author

garronej commented Jan 9, 2024

Let's go @johnksv, you're the man!

@johnksv
Copy link
Contributor

johnksv commented Jan 18, 2024

This issue can be closed since it was merged in #344

@garronej
Copy link
Contributor Author

Yes thanks!

@garronej
Copy link
Contributor Author

Note however that, as of today the region and catalog configuration are parsed as YAML in the values as opposed to onyxia-web where the env configuration are parsed as string (YAML is a superset of JSON).
So to use comment we can just use # ans the trailing slashes where already accepted. Sorry for overseeing this :/

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

No branches or pull requests

4 participants