-
Notifications
You must be signed in to change notification settings - Fork 50
pre-commit hook for formatting JSON files #1256
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Clemens Korner <clemens.korner@gmail.com>
Signed-off-by: Clemens Korner <clemens.korner@gmail.com>
| "u_ref": 1.0, | ||
| "rx_ratio": 6.0, | ||
| "sk": 1e15 | ||
| "sk": 1000000000000000.0 |
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.
This change in the format reduces the readability in my opinion a bit.
But I am not sure if this is a problem.
| - repo: https://github.com/pre-commit/pre-commit-hooks | ||
| rev: v6.0.0 | ||
| hooks: | ||
| - id: pretty-format-json | ||
| args: | ||
| - --autofix # automatically format json files | ||
| - --indent=2 # 2 spaces for indentation | ||
| - --no-sort-keys # when autofixing, retain the original key ordering (instead of sorting the keys) | ||
| - --no-ensure-ascii # preserve unicode characters instead of converting to escape sequences | ||
| files: > | ||
| (?x)^( | ||
| # fix JSON formatting in the following top-level folders: | ||
| code_generation| | ||
| tests | ||
| )/.*\.json$ |
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.
This is the only change in this PR which is not coming from the re-formatting.
If you prefer I can also revert the commit which reformats all the JSON files and you can run pre-commit run --all-files pretty-format-json and commit the changes yourself.
|
Hi @kornerc, Thank you for the contribution. We actually have been thinking about doing this in the past. For some configuration files (e.g. However, for the data files (
This exact reason is why we deliberately use a compact notation with a maximum indentation level (see this formatter and its configuration). That said, having consistent formatting helps with both code quality and code reviews. Here are a couple potential paths going forward:
One last remark: to keep the git commit history clean, can you please either squash the commits after you're done reverting (at the latest when sending it to the merge queue) or open a new PR? |
| hooks: | ||
| - id: markdownlint | ||
| args: ["--fix"] | ||
| - repo: https://github.com/pre-commit/pre-commit-hooks |
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.
can you please also add this to the code quality github actions check?
|
Hi @kornerc, thanks for proposing this new improvement. In addition of what @mgovers said, I would say ignore all PGM data json files. These files already have a custom indent pattern which is produced by the serializer. Also, user/contributor might want to a specific custom indent for a certain data file for some reason (usually modelling or readability). Enforcing a rule on PGM data json files does not make sense. So I would go for option 3 of @mgovers proposed:
|
Changes proposed in this PR include:
testsandcode_generationare formattedpre-commit run --all-files pretty-format-jsonhas been executed to re-format all JSON files.This has been done in a separate commit so that these changes can be easily reverted if formatting options need to be adjusted
I am not sure if this pre-commit hook is wanted.
Feel free to close this PR if this is behavior is not desired.
Could you please pay extra attention to the points below when reviewing the PR:
I hope that this does not cause any unforseen (numerical) issues.
E.g.
10.5e3->10500.01e-5->1e-05