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

JSON formatting using Gson #1125

Merged
merged 14 commits into from Feb 15, 2022
Merged

JSON formatting using Gson #1125

merged 14 commits into from Feb 15, 2022

Conversation

abelk2
Copy link
Contributor

@abelk2 abelk2 commented Feb 13, 2022

This PR adds support for JSON formatting using Google's Gson library.
Motivation:

  • In case of some files (e.g. i18n related ones) alphabetically sorting JSON by keys would be very useful. However, the already existing formatter (called simple, based on org.json) would make this hard to implement, as its internal representation for key-value pairs does not guarantee any order. See the following examples which demonstrate that simple does some kind of key ordering, but not alphabetical:
  • Gson guarantees the same order in its internal representation as in the originally parsed file. Writing that representation back into a file will produce the same order. This way the user can freely choose between a sorted or unsorted (i.e. order left as-is) output - provided that such sorting logic is implemented (which this PR does).
  • Gson has better extensibility and performance than org.json. The former allows us to add more options later to the formatter, see GsonBuilder.

Notes:

  • I kept simple for backwards compatibility.
  • Common test cases for the two implementations have been extracted to a common test class.
  • The only limitation of Gson compared to org.json is that there's no option to leave HTML markup as it was in the original input file. Either all HTML characters are escaped, or none. (I.e. no way to keep it mixed in case the input file was mixed.) This is a minor issue in my opinion, but felt important to share.

Change includes:

  • Gradle support with indentWithSpaces, sortByKeys, escapeHtml, and version properties
  • Unit test updates accordingly, covering added features too
  • Documentation updates

@abelk2 abelk2 marked this pull request as ready for review February 13, 2022 12:38
@abelk2 abelk2 changed the title Feat/gson support JSON formatting using Gson Feb 13, 2022
Copy link
Member

@nedtwigg nedtwigg left a comment

Choose a reason for hiding this comment

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

Great PR, this will definitely get merged in some form, thanks!

Two points:

  1. I think most of the classes in com.diffplug.spotless.json.gson can be made package-private rather than public.

  2. You are currently using lots of reflection. That is fine, and I'm happy to merge this PR as-is. But you might want to consider Use custom source sets and compile-only dependencies instead of reflection #524

@abelk2
Copy link
Contributor Author

abelk2 commented Feb 14, 2022

  1. I think most of the classes in com.diffplug.spotless.json.gson can be made package-private rather than public.

Good point, done. Please note that I had to also put GsonStep into the mentioned package so that it still sees the helper classes. For this, a slight modification was also needed in README.md (only a single package level was supported).

  1. You are currently using lots of reflection. That is fine, and I'm happy to merge this PR as-is. But you might want to consider Use custom source sets and compile-only dependencies instead of reflection #524

Thank you, I completely missed this one. I could definitely deliver this as a future improvement, but would prefer not to with this PR. I'd like to use this on a corporate project as soon as possible.

@nedtwigg
Copy link
Member

We should advertise the compile-only thing better, would #1129 have helped you?

I'd like to use this on a corporate project as soon as possible.

Roger. I'll merge and release once CI completes.

@nedtwigg
Copy link
Member

CI failed with a spotlessApply error ;-)

auto-merge was automatically disabled February 15, 2022 07:19

Head branch was pushed to by a user without write access

@abelk2
Copy link
Contributor Author

abelk2 commented Feb 15, 2022

We should advertise the compile-only thing better, would #1129 have helped you?

Yes, definitely. The contribution guide would be the best place for this info, that's where I looked for it as well.

CI failed with a spotlessApply error ;-)

Oops. sorry for that. Error fixed.

@nedtwigg nedtwigg merged commit 4333885 into diffplug:main Feb 15, 2022
@nedtwigg
Copy link
Member

Published in plugin-gradle 6.3.0.

@ZacSweers
Copy link
Contributor

Out of curiosity, why not just use setPrettyPrinting?

@abelk2
Copy link
Contributor Author

abelk2 commented Feb 20, 2022

@ZacSweers We need to set a custom JsonWriter so that we can customize the indentation size. The prettyPrinting property has no effect on the overload of toJson we use for this, see Gson.java#L835

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.

None yet

3 participants