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 for python formatting #15

Merged
merged 2 commits into from
Nov 24, 2024

Conversation

lukamac
Copy link
Contributor

@lukamac lukamac commented Nov 18, 2024

Add pre-commit config for the python formatting.
Pre-commit makes it easy to add git hooks that check your changes before either committing, pushing, or merging them.
I set the default stage for this config to be pre-push so that it isn't too overbearing.

I didn't add the clang-format because pre-commit doesn't support environment variables and I didn't think of any other clever way to overcome that. We could hardcode the path to the standard llvm installation path (install/llvm) but I don't like that.

I removed the $> from CONTRIBUTING.md so that you can c/p the commands directly to terminal.

@Victor-Jung Victor-Jung requested a review from Xeratec November 18, 2024 22:14
@lukamac lukamac changed the title Add pre-commit for formatting Add pre-commit for python formatting Nov 19, 2024
Copy link
Member

@Xeratec Xeratec left a comment

Choose a reason for hiding this comment

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

I like the idea of adding pre-commit hooks to avoid pushing code that will fail the limiting stage. I have three questions:

  • This requires to install the pre-commit package, right? If so, please add it to dependencies in pyproject.toml
  • Git natively already supports hooks such as pre-commit and pre-push. Why use the pre-commit-hooks and pre-commit? I assume it is because you want to use the packet manager feature of the pre-commit package to ensure that we use the same tools and versions. Additionally, you want to use some of the pre-defined hooks of pre-commit-hooks such as check-added-large-files, correct?
  • Do I need to run pre-commit install to enable these hooks? If so, please explain this in the CONTRIBUTING.md. Additionally, according to the documentation, it is recommended to run pre-commit run --all-files afterward to set up the environment and test the config.

@lukamac
Copy link
Contributor Author

lukamac commented Nov 19, 2024

  • This requires to install the pre-commit package, right? If so, please add it to dependencies in pyproject.toml

Correct, will do.

  • Git natively already supports hooks such as pre-commit and pre-push. Why use the pre-commit-hooks and pre-commit? I assume it is because you want to use the packet manager feature of the pre-commit package to ensure that we use the same tools and versions. Additionally, you want to use some of the pre-defined hooks of pre-commit-hooks such as check-added-large-files, correct?

It's just a convenience to make adding hooks easier. The check-added-large-files was just a 'oooh, that's kinda neat' moment.

  • Do I need to run pre-commit install to enable these hooks? If so, please explain this in the CONTRIBUTING.md.

Yes, you need to run pre-commit install if you'd like the hooks to be installed. I will add it to the CONTRIBUTING.md.

Additionally, according to the documentation, it is recommended to run pre-commit run --all-files afterward to set up the environment and test the config.

Yes, I did that already.

@lukamac
Copy link
Contributor Author

lukamac commented Nov 19, 2024

Plus, it only runs on the changes, not on every file in the project, making it also faster compared to the current instructions in CONTRIBUTING.md

@Xeratec
Copy link
Member

Xeratec commented Nov 21, 2024

  • This requires to install the pre-commit package, right? If so, please add it to dependencies in pyproject.toml

Correct, will do.

After a quick talk with Victor, we agreed that adding it as a package dependency does not make much sense. I suggest you mention that you can install it with pip install pre-commit in CONTRIBUTING.md.

  • Git natively already supports hooks such as pre-commit and pre-push. Why use the pre-commit-hooks and pre-commit? I assume it is because you want to use the packet manager feature of the pre-commit package to ensure that we use the same tools and versions. Additionally, you want to use some of the pre-defined hooks of pre-commit-hooks such as check-added-large-files, correct?

It's just a convenience to make adding hooks easier. The check-added-large-files was just a 'oooh, that's kinda neat' moment.

Indeed, a very nice feature.

  • Do I need to run pre-commit install to enable these hooks? If so, please explain this in the CONTRIBUTING.md.

Yes, you need to run pre-commit install if you'd like the hooks to be installed. I will add it to the CONTRIBUTING.md.

Looks good.

Additionally, according to the documentation, it is recommended to run pre-commit run --all-files afterward to set up the environment and test the config.

Yes, I did that already.

But each user should run this once to set up the environment, right? If so, please add this to CONTRIBUTING.md

Overall, this looks good to me. Just extend the "Pre-commit" in CONTRIBUTING.md with the two points, and we are good to merge. Thanks for your urge to order ;)

@lukamac
Copy link
Contributor Author

lukamac commented Nov 21, 2024

But each user should run this once to set up the environment, right? If so, please add this to CONTRIBUTING.md

Should he? I understood the 4th optional step differently.
I understood it as "when you're adding new hooks to the repository, you should run it on all files because some files might be violating some rules".
Not as "when you're adding new hooks to your local repo".

@lukamac
Copy link
Contributor Author

lukamac commented Nov 21, 2024

@Xeratec added the installation instructions in CONTRIBUTING.md

Copy link
Member

@Xeratec Xeratec left a comment

Choose a reason for hiding this comment

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

Perfect, thanks for the changes. Ready to merge.

@Xeratec Xeratec merged commit 7421833 into pulp-platform:devel Nov 24, 2024
104 checks passed
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