Clang-format in pre-commit hook should test json files too

Beman Libraries
1

clang-format is located in clang/tools/clang-format and can be used to format C/C++/Java/JavaScript/JSON/Objective-C/Protobuf/C# code.

But the pre-commit hook denies this kind of configuration file:

---
# We'll use defaults from the LLVM style, but with 4 columns indentation.
BasedOnStyle: LLVM
IndentWidth: 4
---
Language: Cpp
# Force pointers to the type for C++.
DerivePointerAlignment: false
PointerAlignment: Left
---
Language: JavaScript
# Use 100 columns for JS.
ColumnLimit: 100
---
Language: Proto
# Don't format .proto files.
DisableFormat: true
---
Language: CSharp
# Use 100 columns for C#.
ColumnLimit: 100
...

This would prevent issues like this The current CMakePresets.json file is not a valid json file? · Issue #86 · bemanproject/optional26 · GitHub

2

No objection from me. C++ projects are naturally polyglot in that they contain CMake, JSON, YAML, etc. Picking sensible code formatting across the board is a great idea.

I would like to avoid lots of fiddly configuration per repo, but I’m willing to defer that goal until accessible CI facilities are discovered or developed that meet our needs.

To be specific, I don’t know that we need a few dozen lines in every repo to express “this repo uses Beman standard formatting”. I predict we’ll have subtly different settings across our repos – even with a fairly modest number of repos.

3

It is not caused of formatting, it is to validate the json files too!

And by the way, if we validate yaml file, why keep json files away?

4

What are you suggesting as a fix?

5
  • validate json files this pre-commit
  • use cmake presets on CI
  • change the .clang-format config file to enable JSON
6

validate json files this pre-commit

Assuming you mean adding a pre-commit hook for JSON validation, this sounds reasonable

use cmake presets on CI

I believe there’s an open PR to validate our presets as part of CI

change the .clang-format config file to enable JSON

Sounds reasonable

7 
repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v5.0.0
    hooks:
      - id: trailing-whitespace
      - id: end-of-file-fixer
      - id: check-yaml
        exclude: ^\.clang-format$  # <<<< this helps
      - id: check-added-large-files
8

I would suggest using pre-commit hook for this.

If we only want to validate json files we can simply add:

repos:
  - repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v5.0.0
    hooks:
      - id: trailing-whitespace
      # ...
      - id: check-json

If we want pre-comit to format the json for us we can use:

# ...
- id: pretty-format-json
  args: [--autofix, --no-sort-keys]

This should takecare of everything.

I advise this against clang-format as if someone wants to change linting rules, the first place they will look at would be pre-commit config, people don’t usually think about clang-format when they format json files.

No strong opinion though.

Note that The current CMakePresets.json file is not a valid json file? · Issue #86 · bemanproject/optional26 · GitHub failing suggest inadequate CMake testing more than simple json linting.

9 
index 90da1a9..5bc207f 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -6,7 +6,9 @@ repos:
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer
+      - id: check-json
       - id: check-yaml
+        # exclude: ^\.clang-format$
       - id: check-added-large-files
 
     # Clang-format for C++

this does not solve the the first problem:

pre-commit run check-yaml --files .clang-format --verbose 
check yaml...............................................................Failed
- hook id: check-yaml
- duration: 0.21s
- exit code: 1

expected a single document in the stream
  in ".clang-format", line 3, column 1
but found another document
  in ".clang-format", line 5, column 1

bash-5.2$