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

Pre-commit workflow #21

Merged
merged 10 commits into from
Dec 5, 2023
Merged

Conversation

laserkelvin
Copy link
Contributor

This PR adds a configuration file for pre-commit as the preferred mechanism for maintaining git hooks, replacing the setup_git_hooks.sh and related pre-commit and post-commit execution scripts.

The configuration adds more to the git commit workflow, with some replacements:

Additions

Style/formatting

Style and formatting changes enforce best practices tied to Python 3.9+

  • Trailing whitespace
  • EOF line
  • Pretty JSON formatting
  • Python import ordering and style
  • Black 23.7.0
  • Absolute imports over relative ones

Linting

We replace pylint with flake8, which also includes additional plugins such as flake8-bandit and flake8-black for example.

Security

Adds bandit as part of the workflow which will identify security vulnerabilities with committed code.

Replacements

  • pylint is replaced by flake8
  • autopep8 is replaced by black

Removal

  • unittest was removed for now. pre-commit doesn't have a supported workflow and we could write one ourselves. It might also be a bit extreme to have unit tests run every single commit, and we may want to just have that as CI instead. We can do a follow up PR.

Partially addresses #18 by replacing pylint with flake8 as configured in .pre-commit-config.yaml

Signed-off-by: Lee, Kin Long Kelvin <[email protected]>
Since `setup.py` doesn't have a good mechanism for differentiating
between production and development dependencies, including this file as
a way for contributors

Signed-off-by: Lee, Kin Long Kelvin <[email protected]>
Signed-off-by: Lee, Kin Long Kelvin <[email protected]>
@laserkelvin laserkelvin added the enhancement New feature or request label Nov 30, 2023
@laserkelvin
Copy link
Contributor Author

Some further improvements could be had, if we want to segment into specific git commit stages

Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

CONTRIBUTING.md still refers to pylint and the old hooks. Would you mind reviewing that and updating the text to reflect the changes here?

requirements_dev.txt Outdated Show resolved Hide resolved
@laserkelvin
Copy link
Contributor Author

laserkelvin commented Dec 1, 2023

CONTRIBUTING.md still refers to pylint and the old hooks. Would you mind reviewing that and updating the text to reflect the changes here?

Done in 24728d0. I'll update again with pyproject.toml

@Pennycook
Copy link
Contributor

Done in 24728d0. I'll update again with pyproject.toml

This is good, but I think there are some more parts to change. There are a few references to pylint remaining.

I think we should remove the pre-commit and post-commit bullets from the "Hooks" section, and remove the reference to pylint from "Submitting a Patch". I think the 99 character limit is right (because I think that's the black default) but please double check that too.

@laserkelvin
Copy link
Contributor Author

Done in 24728d0. I'll update again with pyproject.toml

This is good, but I think there are some more parts to change. There are a few references to pylint remaining.

I think we should remove the pre-commit and post-commit bullets from the "Hooks" section, and remove the reference to pylint from "Submitting a Patch". I think the 99 character limit is right (because I think that's the black default) but please double check that too.

black is actually 88 characters - should I update to that?

@Pennycook
Copy link
Contributor

black is actually 88 characters - should I update to that?

I'm not sure. For the P3 Analysis Library we apparently set it to 79:
https://github.com/intel/p3-analysis-library/blob/fec67d5c7f319fbcb4e37d5c7075e055127f4b34/pyproject.toml#L66-L68

Maybe we should just go to 79? It would make it easier to share code between the two projects, which is quite likely.

@laserkelvin
Copy link
Contributor Author

79 is PEP8, we can do that sure

Copy link
Contributor

@Pennycook Pennycook left a comment

Choose a reason for hiding this comment

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

Changes look good. As discussed offline, I don't think we should actually run these hooks until the next release, but I've tested things locally and everything seems to work.

@Pennycook Pennycook merged commit 990100b into intel:main Dec 5, 2023
@Pennycook Pennycook mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants