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

Automatically format the JSON as part of CI #268

Open
Julian opened this issue Jun 30, 2019 · 5 comments
Open

Automatically format the JSON as part of CI #268

Julian opened this issue Jun 30, 2019 · 5 comments
Labels
enhancement An enhancement to the tooling or structure of the suite (as opposed to a new test).

Comments

@Julian
Copy link
Member

Julian commented Jun 30, 2019

See maybe:

https://github.com/typicode/husky

or

https://pre-commit.com/

@Julian Julian added the missing test A request to add a test to the suite that is currently not covered elsewhere. label Sep 27, 2019
@Julian Julian removed the missing test A request to add a test to the suite that is currently not covered elsewhere. label Oct 12, 2019
@jviotti
Copy link
Member

jviotti commented Jun 17, 2022

Another way would be to introduce something very simple like a Makefile to format on demand, and make per-PR checks fail if they are not formatted, asking the user to just run make and re-commit or something like that?

@MeastroZI
Copy link
Contributor

If we format the JSON automatically, there is one drawback. For example, when someone wants to split the changes into 2 commits, all changes get included in one commit, as we are formatting JSON, and running 'git add filename' (or if I have a knowledge gap at this point, please let me know) . This causes all changes to be included in one commit.

So, I think a better option is the same as @jviotti said: we should show an error before 'commit' or 'stage' or on workflow if the file is not formatted properly and provide one command which format all the JSON file

well I am interested to work on this

@gregsdennis
Copy link
Member

gregsdennis commented Apr 27, 2024

I'm not a fan of requiring the committer to do extra work (primarily me being the committer). I much prefer that if anything is done here, it's done as an automatic process where the committer is completely uninvolved.

Secondarily, a git hook only works locally. Any solution that requires a git hook has to be added on everyone's machine.

That leaves one solution: a CI step that runs after merging to main and adds a reformat commit.

We did this on the JSON Path test suite. It's pretty simple.

@jdesrosiers
Copy link
Member

jdesrosiers commented Apr 30, 2024

My concern about automatic formatting is that most of the time it makes code less readable because it's too generic and can't take context and aesthetics into account. The developer can make better aesthetic choices based. For example, a simple reference is best written on one line:

"foo": { "$ref": "#/$defs/bar" }

but an auto formatter will generally split it up:

"foo": {
  "$ref": "#/$defs/bar"
}

The required keyword is similar. Context will determine whether it's nicer for the array to be on one line or on multiple lines and auto formatting eliminates the developer's ability make the aesthetic choice. I often use a blank line to split up logical sections of a schema for readability. An auto formatter will generally remove those empty lines. There are many examples and while each is small, they tend to pile up leaving the result significantly worse that what was hand crafted by the developer.

I am strongly against automation that automatically overrides the aesthetic choices of a developer. I'd like to see the formatter limited only to rules that are universal such as spacing and indentation and not affect a developer's aesthetic choices such as inserting/removing newlines.

Generally I prefer that these things warn the developer what they did that violates the code style and gives them a chance to fix it. Developers will learn the preferred code style better if they make the changes themselves rather than have those changes done for them. If they have to do it themselves, they'll soon stop making the same mistakes. Not having it automatic also allows users to ignore warnings that deliberately violate some rule for aesthetic purposes. However, if everyone prefers auto formatting, I just ask that it be configured to not mess with newlines.

ESLint has a lot of rules that can fairly intelligently allow for developer freedom while keeping reasonable boundaries. They can even auto format based on those rules if that's what we want. However ESLint is designed for JavaScript, not JSON. I know there are ways to use ESLint with JSON, but I don't know if you have access to the same richness of rule expression as you do with JavaScript.

@jdesrosiers
Copy link
Member

This looks promising, https://ota-meshi.github.io/eslint-plugin-jsonc/. It looks like it covers the basics and has at least some of the more advanced rules that I mentioned implemented, such as this one, https://eslint.org/docs/latest/rules/object-curly-newline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement to the tooling or structure of the suite (as opposed to a new test).
Projects
None yet
Development

No branches or pull requests

5 participants