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

JP & TF: pre-commit #77

Merged
merged 9 commits into from
Dec 10, 2021
Merged

JP & TF: pre-commit #77

merged 9 commits into from
Dec 10, 2021

Conversation

fel-thomas
Copy link
Member

@fel-thomas fel-thomas commented Dec 10, 2021

Summary

I'm taking @justinplakoo PR in order to try to detail and split the code so that we can discuss about pre-commit.

This pull request is to introduce the pre-commit mechanism. The goal is simple: once installed, it will make minor fixes for you before committing. I added the dependency in the requirement_dev and it will be enough to install it with a pip install.

For more information: Pre-commit.

Introduction of Pre-commit

In details now, pre-commit are defined as hooks in the pre-commit-config.yaml file. The hooks that are introduced for the moment are:

  • trailing whitespace (4b1e357) which simply remove all the bad whitespace
  • large files (0602a4a) which prevent us to commit large file by mistake
  • ast (b350164) which check whether files parse as valid python
  • docstring first (5487d86) which check a common error of placing code before the docstring
  • merge conflict (af17b3f) which look for merge conflict and prevent us to commit them
  • final newline (4373d0f) which ensure we have a final newline at the end of each file

Finally, we run those checks once in (f509dac).
🙏 I apologize in advance for all the files affected. I took the liberty of checking all the files one by one to make sure there were no aberrations but a second pair of eyes on this commit would not be too much.

These hooks are automatically triggered during the commits. In case you want to trigger them by hand, @justinplakoo has put a specific command in the makefile: make pc_check.

Copyright & Governance

On another note, the governance file made by @justinplakoo is introduced, the existing MIT license where the copyright was in my name is now on the IRT / DEEL / UPS / ANITI (3e77029).

Proposition

I think we should add the pre-commit in the test CI ? What do you think ? If you agree I can prepare another PR to add it.

@fel-thomas fel-thomas self-assigned this Dec 10, 2021
@fel-thomas fel-thomas added the continuous-integration Improvement relative to the CI pipeline label Dec 10, 2021
@fel-thomas fel-thomas changed the title Jp tf/pre commit JP & TF: pre-commit Dec 10, 2021
Copy link
Collaborator

@AntoninPoche AntoninPoche left a comment

Choose a reason for hiding this comment

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

There seems to be no aberration in the white space cleaning and final line adding.

Copy link
Collaborator

@lucashervier lucashervier left a comment

Choose a reason for hiding this comment

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

Nothing to declare

Copy link
Member

@justinplakoo justinplakoo left a comment

Choose a reason for hiding this comment

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

Everything is good for me

@fel-thomas fel-thomas merged commit f1b67b8 into master Dec 10, 2021
@fel-thomas fel-thomas deleted the jp_tf/pre-commit branch December 10, 2021 18:40
@fel-thomas fel-thomas mentioned this pull request Dec 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration Improvement relative to the CI pipeline
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants