Add pre-commit config and GitHub Actions job #1688
Conversation
|
looks like pre-commit needs installing. |
|
This is ready for re-review. Not sure why the buildifier fails in CI, it passes on my machine (TM). Re: integrating clang-format with pre-commit, it seems quite easy, in case you want to restructure / unify the actions: https://stackoverflow.com/questions/55965712/how-do-i-add-clang-formatting-to-pre-commit-hook EDIT: Need to rebase, one second. |
Contains the following hooks: * buildifier - for formatting and linting Bazel files. * mypy, ruff, isort, black - for Python typechecking, import hygiene, static analysis, and formatting. The pylint CI job was changed to be a pre-commit CI job, where pre-commit is bootstrapped via Python. Pylint is currently no longer part of the code checks, but can be re-added if requested. The reason to drop was that it does not play nicely with pre-commit, and lots of its functionality and responsibilities are actually covered in ruff.
nicholasjng
force-pushed
the
pre-commit
branch
from
a9f2103
to
678f9fd
Compare
|
"pylint" is not installed i think |
|
Yeah, I forgot deleting that - most (but not all) of pylint's work is taken over by I also forgot to add the tool configs to the Barring some type annotations, the stuff I'm about to push should complete this. Are you fine with line length 100 for Python files, or do you require a specific different value? |
if it's a swap out for ruff then sure.
sgtm
|
In particular, set line length 80 for all Python files.
Also ignores the `tools/compare.py` and `tools/gbench/report.py` files for mypy, since they emit a barrage of errors which we can deal with later. The errors are mostly related to dynamic classmethod definition.
|
Link to the ruff / pylint comparison: https://docs.astral.sh/ruff/faq/#how-does-ruffs-linter-compare-to-pylint TLDR; it's not a 1:1 but it covers more rules, and especially when including mypy you more than make up for what you lose on static typechecking. Python files are reformatted to length 80 now. |
sorry, python style is also 2 indents. that should also minimise a lot of the changes. |
|
Looking at the style guide, it seems indentations are 4 spaces? (https://google.github.io/styleguide/pyguide.html#s3.4-indentation) I looked at the diff of e.g. It seems to me like |
wow. that's different to the internal version. that'll be fun to integrate later 😅
no i was.
|
|
So regarding Style guide decision is up to you, I don't want to needlessly mess up the repository formatting. Just give me a heads-up on how this should be handled and I'll resubmit accordingly if necessary. |
clang-format: not yet. style guide: follow what the external links say please |
https://google.github.io/styleguide/pyguide.html#s1-background says that "Many teams use the black auto-formatter" (which is configured in this PR), so does that make it eligible to use? From a quick survey of Google Python OSS projects, https://github.com/google/yapf seems to be another popular one - that's also available for setup in pre-commit. |
i have no opinion about the tools for Python, as long as the styling is consistent and matches the google style i'm happy. |
|
This should be ready to review then. From my best-effort research, the Black style is consistent with the public Google Python Style guide. |
|
thank you! |
Contains the following hooks:
The pylint CI job was changed to be a pre-commit CI job, where pre-commit is bootstrapped via Python.
Pylint is currently no longer part of the code checks, but can be re-added if requested. The reason to drop was that it does not play nicely with pre-commit, and lots of its functionality and responsibilities are actually covered in ruff.