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

initial pre-commit config #266

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kbroch-rivosinc
Copy link
Collaborator

soft launch meaning only people with pre-commit tool installed would see this run. (full launch would entail, running pre-commit on all files to clean up past and github action to enforce pre-commit)

If you want to see full effect on files run: pre-commit run --all-files I looked the results of this over and changes seem fine.

There's a lot of formatting changes to the html. (but some of the html looks minified so to be expected) I'd suggest maintainers look over what the changes would be for all the files and the generate final docs.

fixes #260

@dhower-qc
Copy link
Collaborator

check out #270, which adds "./bin/pre-commit" to run pre-commit regardless of whether you are using a devcontainer or singularity

@dhower-qc
Copy link
Collaborator

we should ignore everything in docs/ruby. those are all generated files

@dhower-qc
Copy link
Collaborator

Can pre-commit check for Unicode characters? A common problem is when a cut/paste from, e.g., the ISA manual, brings in Unicode to the YAML. That sometimes messes with the tools.

@kbroch-rivosinc
Copy link
Collaborator Author

Can pre-commit check for Unicode characters? A common problem is when a cut/paste from, e.g., the ISA manual, brings in Unicode to the YAML. That sometimes messes with the tools.

The default check-yaml https://github.com/pre-commit/pre-commit-hooks/blob/main/pre_commit_hooks/check_yaml.py#L11 does a load of the yaml file. Would this detect that problem?

@dhower-qc
Copy link
Collaborator

It might, but I'm not sure. The problem arises when we try to load a YAML file with non-ASCII characters using the standard Ruby YAML library (which itself is just a wrapper around the C libyaml library). It will fail, but unfortunately without a helpful error message.

I see that the library used by pre-commit -- ruamel.yaml -- has some pretty cool features like key order preservation and comment preservation. It could be a part of a solution to the "scripting changes" problem I've been concerned about.

It's easy enough to make a little script to check for Unicode. Probably easier to do that then search around for something turn-key ready. How about this: I'll make the script and you figure out how to run it from pre-commit?

@kbroch-rivosinc
Copy link
Collaborator Author

It might, but I'm not sure. The problem arises when we try to load a YAML file with non-ASCII characters using the standard Ruby YAML library (which itself is just a wrapper around the C libyaml library). It will fail, but unfortunately without a helpful error message.

I think we might need to discuss this more. My concern is YAML and AsciiDoc are both fully capable of supporting unicode and given these are the "docs" putting the ascii only restriction on the content seems problematic, especially if we want to support tooling that could be used to generate docs in other languages.

@dhower-qc
Copy link
Collaborator

Good point. Maybe I'll just figure out why it's failing ;) It should be able to handle UTF-8, after all...

@kbroch-rivosinc
Copy link
Collaborator Author

@dhower-qc : this PR should be good to go as we discussed. Broke up into 2 commit: one for hooks added, one for autofix changes from those hooks

@dhower-qc
Copy link
Collaborator

When I try this, pre-commit doesn't seem to do anything:

hu-dhower-lv:riscv-unified-db-3 (65) ./bin/pre-commit 
[INFO] Initializing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Installing environment for https://github.com/pre-commit/pre-commit-hooks.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
check for broken symlinks............................(no files to check)Skipped
fix end of files.....................................(no files to check)Skipped
trim trailing whitespace.............................(no files to check)Skipped

Same with pre-commit run --all-files. Is this the expected output?

@kbroch-rivosinc
Copy link
Collaborator Author

Same with pre-commit run --all-files. Is this the expected output?

No after you pulled in 2f735f8 you should have seen this:

❯ pre-commit run --all-file
check for broken symlinks............................(no files to check)Skipped
fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing arch/csr/mhartid.yaml
Fixing arch/csr/mip.yaml
Fixing arch/isa/fp.idl

trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing schemas/csr_schema.json
Fixing lib/test/test_yaml_loader.rb
Fixing arch/csr/mip.yaml
Fixing arch/csr/mie.yaml

But I'm not running pre-commit from the container.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add automation for consistent formatting and linting
2 participants