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

Sort by keys inside json arrays #1546

Merged
merged 6 commits into from
Feb 1, 2023

Conversation

genuss
Copy link

@genuss genuss commented Jan 31, 2023

Hi team!
I'd like to propose an improvement of JSON formatting feature implemented in #1125
Now, if sortByKeys is enabled, objects are recursively sorted but arrays are skipped. Take a look at this JSON for example:

{
  "array": [
    {
      "b": "b",
      "a": "a"
    }
  ]
}

Note that keys are not sorted now and will not be sorted after applying the formatter. My proposal is to sort these keys too.
I'm not sure whether existing configuration property should be reused as this changes the behaviour of GSON formatter. If it's not acceptable I can add another property like sortByKeysInArrays to preserve backwards compatibility. When we choose which one is better, I'll certainly add some tests.
Let me know, what do you think about it.

@genuss genuss marked this pull request as ready for review January 31, 2023 12:31
@nedtwigg
Copy link
Member

I think the behavior you have implemented is the correct behavior, and the old behavior was a bug. Labelling it as "potentially breaking" is perfect, alerts people who may have come to rely on the bug.

If anyone wants to add a flag in the future "sortByKeysExceptArrayElements" to restore the previous behavior, I would merge that PR so that they could keep the old behavior.

To get merged, this PR needs

@genuss
Copy link
Author

genuss commented Jan 31, 2023

Thanks for comments, a key to restore the old behaviour is even better! Will do everything tomorrow and send an update.

@genuss
Copy link
Author

genuss commented Feb 1, 2023

All done. As we consider this as a bug, I didn't add a new test but modified the existing one.

@nedtwigg
Copy link
Member

nedtwigg commented Feb 1, 2023

Tests are failing. Looks like the plugin integration tests don't like the changes.

@genuss
Copy link
Author

genuss commented Feb 1, 2023

Indeed I didn't run integration tests. Fixed and reran everything locally, it should pass now.

@nedtwigg nedtwigg merged commit fd40aa7 into diffplug:main Feb 1, 2023
@genuss genuss deleted the feature/json-sort-arrays branch February 2, 2023 09:01
@nedtwigg
Copy link
Member

nedtwigg commented Feb 5, 2023

Implemented in plugin-gradle 6.14.1 and plugin-maven 2.32.0.

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