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 GitHub action to format and lint code #96

Merged
merged 31 commits into from
Sep 6, 2023
Merged

Conversation

dyastremsky
Copy link
Contributor

@dyastremsky dyastremsky commented Jun 23, 2023

This pull request adds a pre-commit to run the following:

  • Black for formatting Python
  • Flake8 for linting Python for conformance to PEP8, PyFlakes, and circular complexity (max-line-length set to match Black's 88 default/recommended character limit; this and other settings from Black's documentation here to avoid enforcement of limit when unnecessary).
  • isort for deterministically sorting and organizing Python imports
  • Clang for formatting C++, C, Java, TextProto, Proto, and CUDA
  • Codespell for finding common spelling mistakes

In addition, this adds these native pre-commit hooks, details here:

  • check-case-conflict
  • check-executables-have-shebangs
  • check-merge-conflict
  • check-json
  • check-toml
  • check-yaml
  • check-shebang-scripts-are-executable
  • end-of-file-fixer
  • mixed-line-ending
  • requirements-txt-fixer
  • trailing-whitespace

To run these locally, you can install pre-commit via pip install pre-commit, then go into the repo and run pre-commit install. After that has been done once, you just need to run pre-commit run --all-files any time you want to apply them. Once installed, it should run automatically on commit for files included in a commit. To correct spelling errors found by codespell when possible, you can install codespell (pip install codespell and potentially pip install --upgrade codespell to use the toml file in the directory) and call codespell -w <path>.

This pull request also adds a GitHub action so that these are run for every pull request and push to main. The core changes are in pre-commit.yml, pre-commit-config.yaml, and pyproject.toml. This also removes the old formatter (.clang-format, tools/format.py, tools/pre-commit). The rest of the changes are the effect of applying the action to the current repo.

Note: When this is merged, we'll want to modify the CONTRIBUTING document in the server repository at the same time to update the instructions on running the formatter.

@dyastremsky dyastremsky marked this pull request as ready for review June 23, 2023 17:39
Copy link

@csadorf csadorf left a comment

Choose a reason for hiding this comment

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

Very nice! Just a few minor comments.

You mention in the PR description that it would be advisable to adjust the CONTRIBUTING.md. I agree, but why not modify the file directly in this PR?

pyproject.toml Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@dyastremsky
Copy link
Contributor Author

@csadorf Thanks for your comments! The rationale for not updating the CONTRIBUTING document here is because it's in the triton-inference-server/server repo. When we roll these changes out to all repos next week, we'll update the CONTRIBUTING document in the server repo in the same PR.

@@ -34,4 +35,4 @@ BinPackArguments: true
BinPackParameters: true
ConstructorInitializerAllOnOneLineOrOnePerLine: false

IndentCaseLabels: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be a newline. :) I think Git only shows the lack of a newline (see red/removed area vs. green/added area).

@fpetrini15 fpetrini15 self-requested a review September 6, 2023 22:01
Copy link
Contributor

@fpetrini15 fpetrini15 left a comment

Choose a reason for hiding this comment

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

I was going to ask whether adding "black" as a profile in the pyproject.toml would impact the other PRs, but it looks like you've already updated them. Great work!

@dyastremsky dyastremsky merged commit 468eb21 into main Sep 6, 2023
1 check passed
@dyastremsky dyastremsky deleted the dyas-precommit branch September 6, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants