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

Chore - Pre-commit #164

Merged
merged 3 commits into from
Sep 7, 2024
Merged

Conversation

cusma
Copy link
Contributor

@cusma cusma commented Sep 6, 2024

Intro

Hi all! 👋

This is Cosimo! MSc in Electrical Engineering at Politecnico di Torino. ⚡️

It is my first contribution on the repository. This PR supersedes #149 and #160.

Scope

This PR is not touching any "core" electrical engineering feature, it is just improving the overall quality of the codebase and of the open source project.

The PR adds pre-commit hooks and GH action to the repository:

Git hook scripts are useful for identifying simple issues before submission to code review. We run our hooks on every commit to automatically point out issues in code such as missing semicolons, trailing whitespace, and debug statements. By pointing these issues out before code review, this allows a code reviewer to focus on the architecture of a change while not wasting time with trivial style nitpicks.

To run pre-commit:

  1. Follow the install guide
  2. Execute pre-commit run -a in the folder containing the .pre-commit-config.yaml file

Git hooks

I've added the following git hooks to the .pre-commit-config.yaml:

  • Pip Audit: Audits Python environments and dependency trees for known vulnerabilities
  • Generic:
    • Ensures that a file is either empty, or ends with one newline
    • Trims trailing whitespace
  • YAML: Checks .yaml files for parseable syntax (again, in this repo would target mostly GH actions and pre-commit config)
  • TOML: Checks .toml files for parseable syntax
  • YAPF: Python code formatter

GH Action

A pre-commit.yml GH actions is added to include pre-commit validation in the CI pipeline.

Automatic changes

⚠️ All the PR diff is autogenerated according code formatting rules.

@mhinkkan mhinkkan merged commit b6d8365 into Aalto-Electric-Drives:main Sep 7, 2024
1 check passed
@mhinkkan
Copy link
Member

mhinkkan commented Sep 7, 2024

@cusma, it seems to me that trimming trailing white spaces does not work as expected, i.e., they are not automatically removed during the commit process (what was my expectation). Instead, the pre-commit process fails if there are trailing white spaces in the files. Any views on this?

@cusma
Copy link
Contributor Author

cusma commented Sep 7, 2024

@cusma, it seems to me that trimming trailing white spaces does not work as expected, i.e., they are not automatically removed during the commit process (what was my expectation). Instead, the pre-commit process fails if there are trailing white spaces in the files. Any views on this?

@mhinkkan did you install pre-commit and run:

pre-commit run -a

Before pushing the commit? It should validate and fix the code according the formatting and linting rules automatically.

Than the pre-commit.yml actions in the continuous integration pipeline safeguards the protected branches (e.g. main) from "broken" commits (with respect the validation rules defined in .pre-commit-config.yaml).

@cusma
Copy link
Contributor Author

cusma commented Sep 7, 2024

Also I described the process in the PR body, but I will open a PR to add this small step to the README and the contributing guidelines so the automatic formatting and linting process is clear for all the contributors.

@mhinkkan
Copy link
Member

mhinkkan commented Sep 7, 2024

@cusma: Instead of installing and running pre-commit locally, would it be simpler to run it automatically at GitHub instead (GitHub actions or CI, I'm not too familiar with these...)?

https://stackoverflow.com/collectives/articles/71270196/how-to-use-pre-commit-to-automatically-correct-commits-and-merge-requests-with-g

https://pre-commit.ci/lite.html

@cusma
Copy link
Contributor Author

cusma commented Sep 7, 2024

@cusma: Instead of installing and running pre-commit locally, would it be simpler to run it automatically at GitHub instead (GitHub actions or CI, I'm not too familiar with these...)?

https://stackoverflow.com/collectives/articles/71270196/how-to-use-pre-commit-to-automatically-correct-commits-and-merge-requests-with-g

https://pre-commit.ci/lite.html

It could be done, but it would require installing an external GitHub Application (e.g. pre-commit.ci) in the repository (which I have no authorization to), which I felt a bit invasive (also because that was my first contribution).

In my opinion, the combination of running pre-commit hooks locally and check PR's quality in remote with GitHub actions (CI pipeline) is a pretty standard process, as people tend to avoid autogenerated code changes from bots/apps that are not approved/signed by developers (a part from dependencies update proposals, like Dependabot).

Running pre-commit automatically in remote kind goes against the design and purpose of pre-commit, which is supposed really to just be installed locally (once, in a cached and project-isolated environment) and invoked automatically (with git hooks) before commits.

Anyway, I defer this decision to you as project author and maintainer, happy to help either way.


P.S.
As a side note, to be familiar with those (confusing) acronyms and jargon: GitHub Actions are the automatic processes running on GitHub and specified in the .github/workflows folder (either manually or from published ones). GitHub Actions are a way to implement Continuous Integration and Continuous Delivery (CI/CD) development methodology.

@cusma cusma deleted the chore/pre-commit branch September 7, 2024 16:11
@mhinkkan
Copy link
Member

mhinkkan commented Sep 8, 2024

Thanks, let's keep pre-commit local at this point. I added it to optional dev dependencies and mentioned it in the contributing guidelines' workflow.

@all-contributors please add @cusma for infra

Copy link
Contributor

@mhinkkan

I've put up a pull request to add @cusma! 🎉

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.

2 participants