-
Notifications
You must be signed in to change notification settings - Fork 456
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
Conversation
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
271477f
to
d15850d
Compare
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
d15850d
to
7aec76d
Compare
7aec76d
to
26c04fd
Compare
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JsonExtension.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
ffbc5dc
to
ec624c0
Compare
ec624c0
to
1287b3d
Compare
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
lib/src/main/java/com/diffplug/spotless/json/JsonFormatterStep.java
Outdated
Show resolved
Hide resolved
plugin-gradle/src/main/java/com/diffplug/gradle/spotless/JsonExtension.java
Outdated
Show resolved
Hide resolved
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 🤔 |
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. |
44866e6
to
cab805e
Compare
Looks like it's still not happy, I'm seeing:
And given it's pretty close to what is used in i.e. |
plugin-gradle/src/test/java/com/diffplug/gradle/spotless/JsonExtensionTest.java
Outdated
Show resolved
Hide resolved
Hey @nedtwigg was wondering if you or any of the team could spot why this doesn't seem to be working? |
At least right now, the tests are failing because the PR needs a Because of that I can't see a test failure stacktrace in CI. Seems unlikely but possible that 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
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 |
plugin-gradle/src/test/java/com/diffplug/gradle/spotless/JsonExtensionTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
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.
Thanks Ned - good points for those documentation tweaks 👍 |
Published in |
Hi, Is this available in plugin-maven as well?. |
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 :) |
Any one know if this (along with #910) will be merged/released soon? |
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 |
@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. |
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 aproject 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 explicitdependency.
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:
SomeNewStep
.create
that returns aFormatterStep
.SomeNewStepTest
.equality()
which tests equality usingStepEqualityTester
(see existing methods for examples).CHANGES.md
-SNAPSHOT
section of CHANGES.md, plugin-gradle/CHANGES.md, and plugin-maven/CHANGES.md which includes: