Pre-commit check failing on new JSON file and doesn't report useful line number

[james-ball-qualcomm]

In PR #358 the pre-commit check fails on a new JSON file I created:

1. Why is the checker requiring an empty line at the end of the file? Is this a JSON requirement? If not, seems like an unnecessary check since JSON parsers shouldn't require this and VSCode is happy with it (the only reader of this JSON file).
2. The second error is ".vscode/launch.json: Failed to json decode (Expecting property name enclosed in double quotes: line 2 column 3 (char 4))". If you look at the JSON file, line 2 is a comment at the beginning of the file. Either the checker is parsing comments (which it shouldn't) or doesn't report the correct location of the error in the JSON file. So, this seems like we need to improve the checker or disable this check.

[dhower-qc]

JSON doesn't support comments. launch.json is actually "JSON with comments", a different spec. You just need to update .pre-commit-config.yaml to exclude what's in .vscode

[kbroch-rivosinc]

explanation for

1. https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
   So not JSON specific, just a good convention to follow for all text files.

[james-ball-qualcomm]

I can just remove the comments since VSCode put those in automatically and they aren't required.

[james-ball-qualcomm]

Do we have any tools that fail if there isn't an empty line at the end of a text file? I've been in the industry for many years and never had this as a requirement in any file and any tool I've ever used doesn't require an empty line at the end of a file. So, why be burdened by a non-requirement of all the UDB developers to know that all text files must have an empty line at the end? Seems too restrictive and not a good ROI.

[james-ball-qualcomm]

Why doesn't the pre-commit check rule run when I do a commit in VSCode? Derek just told me it was supposed to so something seems broken there.

[james-ball-qualcomm]

Let's not make this pre-commit check too restrictive and annoy our developers. Recently Rafael added a check that you couldn't have trailing whitespace on a line and that created all sorts of problems since many languages allow this. Let's not repeat that situation with UDB.

[dhower-qc]

Why doesn't the pre-commit check rule run when I do a commit in VSCode? Derek just told me it was supposed to so something seems broken there.

It's only going to check files that you staged in the commit, not everything.

[dhower-qc]

Let's not make this pre-commit check too restrictive and annoy our developers. Recently Rafael added a check that you couldn't have trailing whitespace on a line and that created all sorts of problems since many languages allow this. Let's not repeat that situation with UDB.

I'm in favor of keeping the rule. It helps to avoid subtle errors when processing/transforming files, which is something we do (and will continue to do) a lot of since our data is in text files.

[dhower-qc]

Further, your experience should be an outlier; New checkouts (or after doing ./do clean) will have pre-commit automatically installed as a git hook, so you'll get the files corrected for you when you commit locally -- you should never have to deal with pre-commit flagging something remotely on GitHub)

[james-ball-qualcomm]

The JSON file in question was staged and then committed so why isn't the pre-commit check running for me?

[james-ball-qualcomm]

I just did a "git fetch" and then a merge from main into a branch I'm getting ready to do a PR. Now I'm seeing this:

james@JAMEBALL-P1:/github/riscv-unified-db$ ./do clean
Traceback (most recent call last):
File "/github/riscv-unified-db/.home/.venv/bin/pre-commit", line 5, in
from pre_commit.main import main
ModuleNotFoundError: No module named 'pre_commit'
james@JAMEBALL-P1:/github/riscv-unified-db$

[james-ball-qualcomm]

Okay, what is this all about? Now I've got some ambiguous pre-commit failures on a completely different PR. Why are we doing trailing whitespace checks? If a tool can't handle a text file without an empty line at the end, can't we fix the tool and don't burden everyone who has to edit a text file. Tools should conform to people, not the other way around.

https://github.com/riscv-software-src/riscv-unified-db/actions/runs/12385005005/job/34570520091?pr=341