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

Add pre-commit support #1294

Merged
merged 2 commits into from
Dec 22, 2022
Merged

Add pre-commit support #1294

merged 2 commits into from
Dec 22, 2022

Conversation

abejgonzalez
Copy link
Contributor

@abejgonzalez abejgonzalez commented Dec 21, 2022

See firesim/firesim#1329 for original PR.

This PR adds pre-commit support (https://pre-commit.com/) to run pre-commit hooks. Look at the .pre-commit-config.yaml file to see how pre-commit hooks are enabled. For a list of other supported plugins see https://pre-commit.com/hooks.html. To enable the hooks you have to run pre-commit install so I added that command to the env.sh file (this adds a small lag - ~O(s) - to the 1st time the env.sh is invoked but afterwards is a noop) (see build-setup.sh). Additionally, to run the hooks on all files in the repo I ran pre-commit run --all-files. Normally only the files that are committed are checked with the pre-commit hooks.

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

@abejgonzalez abejgonzalez self-assigned this Dec 21, 2022
@jerryz123 jerryz123 requested review from jerryz123 and removed request for jerryz123 December 21, 2022 21:37
@jerryz123
Copy link
Contributor

Neat. What happens when you break one of the rules? Does it fix your commit for you?

@tymcauley
Copy link
Contributor

It looks like it edited some .svg files. Is that bad? Seems like it shouldn't search for whitespace issues in a binary file.

@jerryz123
Copy link
Contributor

SVGs are XML files, interestingly.

@tymcauley
Copy link
Contributor

SVGs are XML files, interestingly.

🤯

@abejgonzalez abejgonzalez changed the title Add initial pre-commit support Add pre-commit support Dec 21, 2022
@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Dec 21, 2022

If there is a breakage it can either warn you and exit (for example for incorrect yaml) or edit the files itself (adding the newline/deleting the extra spaces).

@abejgonzalez
Copy link
Contributor Author

I also just enabled all the pre-commit hooks since I think there isn't much movement in the repo right now. So now could be a good time to just force formatting on everything.

@abejgonzalez
Copy link
Contributor Author

For reference on the Clang formatting: https://zed0.co.uk/clang-format-configurator/

@abejgonzalez
Copy link
Contributor Author

abejgonzalez commented Dec 21, 2022

Note that the documentation error is not due to this PR (the BOOM documentation / website is down for some reason) - This should be fixed in the next few hours / I had to re-buy the domain.

vlsi/example-vlsi Outdated Show resolved Hide resolved
vlsi/view_gds.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like forcing formatting on source code like this. No-trailing-whitespace is fine, but restructuring code and comments like this is too heavy-handed.

@abejgonzalez
Copy link
Contributor Author

I personally think having a linter/code-formatter is better than not having one at all, but we can push that to a later discussion. For now this PR will just focus on whitespace management + checking files.

@abejgonzalez
Copy link
Contributor Author

@jerryz123 This is now ready for rereview

Copy link
Contributor

@jerryz123 jerryz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing all the trailing-whitespace/newline issues. The git hooks thing seems useful.

@abejgonzalez abejgonzalez merged commit 6904426 into main Dec 22, 2022
@jerryz123 jerryz123 deleted the pre-commit branch February 13, 2023 07:52
@abejgonzalez abejgonzalez mentioned this pull request Aug 28, 2023
16 tasks
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.

3 participants