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: add pre-commit hooks #2324

Merged
merged 17 commits into from
Apr 23, 2021
Merged

chore: add pre-commit hooks #2324

merged 17 commits into from
Apr 23, 2021

Conversation

Yun-Kim
Copy link
Contributor

@Yun-Kim Yun-Kim commented Apr 16, 2021

Description

In order to automate linting and formatting tools including black, flake8, isort, and mypy and pre-commit tools including EOF and whitespace trimming and YAML file checking, a pre-commit hook configuration was set up.

This was initially done with the pre-commit package, but it did not allow synchronization with our riot tool versions. Thus, we moved to using autohook which is a simple shell script that runs our riot commands and lets us avoid having to maintain additional versions.

Note: in order to use these pre-commit hooks, the following command must be run once beforehand:
pre-commit install hooks/autohook.sh install. This has been addressed by adding the riot command riot run pre-commit riot run autohook which can be run once and installs the pre-commit autohook pre-commit script and initializes the riot hooks to be run on git commit commands.

@Yun-Kim Yun-Kim added the changelog/no-changelog A changelog entry is not required for this PR. label Apr 16, 2021
@Yun-Kim Yun-Kim requested a review from a team as a code owner April 16, 2021 20:24
.pre-commit-config.yaml Outdated Show resolved Hide resolved
riotfile.py Outdated Show resolved Hide resolved
riotfile.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 22, 2021

Codecov Report

Merging #2324 (1a54f21) into master (d411dee) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2324   +/-   ##
=======================================
  Coverage   91.22%   91.22%           
=======================================
  Files         573      573           
  Lines       41239    41256   +17     
=======================================
+ Hits        37621    37637   +16     
- Misses       3618     3619    +1     
Impacted Files Coverage Δ
ddtrace/contrib/urllib3/patch.py 100.00% <100.00%> (ø)
tests/contrib/urllib3/test_urllib3.py 99.63% <100.00%> (+0.02%) ⬆️
ddtrace/profiling/exporter/http.py 98.91% <0.00%> (-1.09%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8b33989...1a54f21. Read the comment docs.

brettlangdon
brettlangdon previously approved these changes Apr 22, 2021
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👍 any reason for not including isort?

hooks/autohook.sh Show resolved Hide resolved
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

🙌 can't wait to install these!

@mergify mergify bot merged commit a90037e into DataDog:master Apr 23, 2021
@Yun-Kim Yun-Kim deleted the pre-commit-hook branch April 23, 2021 14:03
@jd
Copy link
Contributor

jd commented Apr 26, 2021

What's the rationale for using a bash script rather than the pre-commit tool?

@Yun-Kim
Copy link
Contributor Author

Yun-Kim commented Apr 26, 2021

This was initially done with the pre-commit package, but it did not allow synchronization with our riot tool versions. Thus, we moved to using autohook which is a simple shell script that runs our riot commands and lets us avoid having to maintain additional versions.

@jd pre-commit IIRC unfortunately works by cloning github repos to grab the formatting tools and require versions to be specified, this would result in us having to maintain additional versions to be in sync with our riotfile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants