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

Add JVM-based JSON formatter #853

Merged
merged 3 commits into from
Jun 17, 2021
Merged

Conversation

jamietanna
Copy link
Contributor

@jamietanna jamietanna commented Apr 25, 2021

Add JVM-based JSON formatter

Currenly, if consumers of Spotless want to format their JSON files, they
need to do this by adding prettier, which increases the dependencies a
project needs.

To simplify things for consumers, as documented in #850, we can provide
a JVM-based JSON formatter, using org.json's JSON formatting.

To follow how we've done this with other formatters, we can retrieve
org.json's JAR at runtime, and retrieve the classes via Reflection,
instead of adding this as a lib-extra project, with an explicit
dependency.

To validate this fully, we want a few straightforward JSON files to
validate before/after, but we can also use a sample Cucumber JSON
report
as a much more complex example of what happens, which we've
removed any data objects from its source, so the files are smaller.

We also want our consumers to be able to configure the indentation size.

Closes #850.


Raising this as an early draft PR for feedback if possible!

I've also tested this against a project of mine that has a lot of JSON files, and it works nicely 😄


TODOs:

  • Class name ends in Step, SomeNewStep.
  • Class has a public static method named create that returns a FormatterStep.
  • Has a test class named SomeNewStepTest.
  • Test class has test methods to verify behavior.
  • Test class has a test method equality() which tests equality using StepEqualityTester (see existing methods for examples).
  • PR allows edits from maintainers
  • Git history
  • Gradle integration
  • CHANGES.md
  • After creating the PR, please add a commit that adds a bullet-point under the -SNAPSHOT section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes:
    • a summary of the change
    • either
      • a link to the issue you are resolving (for small changes)
      • a link to the PR you just created (for big changes likely to have discussion)

@jamietanna jamietanna force-pushed the feature/json branch 3 times, most recently from 271477f to d15850d Compare April 25, 2021 16:54
@jamietanna jamietanna changed the title WIP: JSON formatter Add JVM-based JSON formatter Apr 25, 2021
@jamietanna jamietanna marked this pull request as ready for review April 25, 2021 17:00
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.

Thanks, this is pretty close to mergeable.

@jamietanna jamietanna force-pushed the feature/json branch 3 times, most recently from ffbc5dc to ec624c0 Compare April 28, 2021 12:25
@jamietanna jamietanna marked this pull request as draft April 28, 2021 12:25
@jamietanna
Copy link
Contributor Author

Thanks, that's super helpful! Would we say tabs are a requirement for JSON btw? I know they're offered for other means, but I can't think of any time I've seen them as JSON is usually space pretty-printed 🤔

@nedtwigg
Copy link
Member

Tabs are definitely not a requirement to merge, I just want to make it easy for some someone else to add them later if they want.

@jamietanna
Copy link
Contributor Author

Looks like it's still not happy, I'm seeing:


* What went wrong:
Could not determine the dependencies of task ':spotlessApply'.
> Could not create task ':spotlessJsonApply'.
   > Could not create task ':spotlessJson'.
      > Cannot invoke method simple() on null object

And given it's pretty close to what is used in i.e. KotlinExtension I'm not sure where to go from here 😓

@jamietanna
Copy link
Contributor Author

Hey @nedtwigg was wondering if you or any of the team could spot why this doesn't seem to be working?

@nedtwigg
Copy link
Member

At least right now, the tests are failing because the PR needs a spotlessApply. https://app.circleci.com/pipelines/github/diffplug/spotless/1060/workflows/c9ee7f59-a168-4836-8f02-88809ebc3730/jobs/5994

Because of that I can't see a test failure stacktrace in CI. Seems unlikely but possible that json is some kind of keyword or special operator, maybe try json2 just to test? If that is the problem it might be best for the example code to just be format 'json', com.diffplug.spotless.JsonExtension.class { (I got the package wrong here, but you get the idea).

This is a useful feature, but I throttle my time on Spotless. I won't pull this locally and dig around, and I don't trust stacktraces that I can't verify for myself in our CI setup. I can be more helpful if you can reproduce whatever you are seeing locally in the public CI.

Currenly, if consumers of Spotless want to format their JSON files, they
need to do this by adding `prettier`, which increases the dependencies a
project needs.

To simplify things for consumers, as documented in diffplug#850, we can provide
a JVM-based JSON formatter, using `org.json`'s JSON formatting.

To follow how we've done this with other formatters, we can retrieve
`org.json`'s JAR at runtime, and retrieve the classes via Reflection,
instead of adding this as a `lib-extra` project, with an explicit
dependency.

To validate this fully, we want a few straightforward JSON files to
validate before/after, but we can also use [a sample Cucumber JSON
report] as a much more complex example of what happens, which we've
removed any `data` objects from its source, so the files are smaller.

We also want our consumers to be able to configure the indentation size.

We're calling this a `simple` formatter as it doesn't allow much to be
configured other than the indentation size in spaces.

[a sample Cucumber JSON report]: https://github.com/damianszczepanik/cucumber-reporting/raw/master/src/test/resources/json/sample.json
@jamietanna
Copy link
Contributor Author

Thanks Ned - I appreciate the help! I've added some extra test cases to show the various bits I've tried, which shows a bit more what I've been seeing.

Let me know if you'd like --stacktrace --info added for more logging detail.

@jamietanna jamietanna marked this pull request as ready for review June 16, 2021 16:48
@jamietanna jamietanna requested a review from nedtwigg June 16, 2021 16:48
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.

Very close! Just some docs changes.

plugin-gradle/README.md Outdated Show resolved Hide resolved
plugin-gradle/README.md Outdated Show resolved Hide resolved
As we've created a JVM JSON formatter as part of diffplug#850, we should make it
possible to use it natively in Gradle, which requires we add it as a new
supported type in `SpotlessExtension`.

When configured, we'll default to including any JSON files under the
`src` directory, while allowing it to be overriden if requested.
@jamietanna
Copy link
Contributor Author

Thanks Ned - good points for those documentation tweaks 👍

@jamietanna jamietanna requested a review from nedtwigg June 16, 2021 21:24
@nedtwigg nedtwigg merged commit 8b2f871 into diffplug:main Jun 17, 2021
@nedtwigg
Copy link
Member

Published in plugin-gradle 5.14.0.

@jamietanna jamietanna deleted the feature/json branch June 17, 2021 06:37
@elakkss
Copy link

elakkss commented Nov 15, 2021

Hi, Is this available in plugin-maven as well?.

@jamietanna
Copy link
Contributor Author

I don't see it there (yet) - I don't use Maven so didn't contribute it, but I can see about getting a PR raised at some point in the next few weeks, or if you'd like to pick it up, feel free :)

@talios
Copy link

talios commented Nov 10, 2022

Any one know if this (along with #910) will be merged/released soon?

@jamietanna
Copy link
Contributor Author

Hey talios, do you mean the Gradle or Maven plugin? Gradle been live for a while, the Maven and YAML changes won't be done at least by me any time soon sorry

@talios
Copy link

talios commented Nov 11, 2022

@jamietanna Ahh I was meaning the maven plugin - if it's just a matter of wiring things up from library->mojo I could probably take a look at that ca814b4 commit and see what's up.

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.

Add JVM-based JSON formatter
4 participants