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 configuration #126

Merged
merged 21 commits into from May 4, 2021
Merged

Add pre-commit configuration #126

merged 21 commits into from May 4, 2021

Conversation

joaander
Copy link
Member

@joaander joaander commented May 3, 2021

Description

Replace existing style checks with pre-commit configuration file.

Motivation and context

Pre-commit is popular.

Using fresnel as a test before making the change in HOOMD.

Checklist:

@joaander joaander mentioned this pull request May 3, 2021
3 tasks
@joaander joaander requested a review from bdice May 3, 2021 18:51
@joaander joaander marked this pull request as ready for review May 3, 2021 18:51
@joaander joaander requested a review from a team as a code owner May 3, 2021 18:51
@joaander joaander requested review from tcmoore3 and removed request for a team May 3, 2021 18:51
@joaander
Copy link
Member Author

joaander commented May 3, 2021

@bdice I adapted your pre-commit configuration for HOOMD and tested on fresnel (I wanted to test on a smaller repository where I could run the formatters first). Have a look.

I switched to using conda to install clang-format and combined it into the main pre-commit configuration file using a manual trigger. #127 applies the autoformatters.

Copy link
Member

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Nice! I had a couple minor questions but otherwise LGTM.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.github/workflows/style_check.yml Outdated Show resolved Hide resolved
doc/style.rst Outdated Show resolved Hide resolved
@joaander
Copy link
Member Author

joaander commented May 4, 2021

Well, so much for the manual configuration. pre-commit.ci runs those as well, which fails because the runner doesn't hae conda: https://results.pre-commit.ci/run/github/147663951/1620140732.FzOJofjYSs-tTNYab50BHw

I'll move that config to a separate file as you do in freud for now.

@bdice
Copy link
Member

bdice commented May 4, 2021

Well, so much for the manual configuration. pre-commit.ci runs those as well, which fails because the runner doesn't hae conda: https://results.pre-commit.ci/run/github/147663951/1620140732.FzOJofjYSs-tTNYab50BHw

I'll move that config to a separate file as you do in freud for now.

@joaander You can skip it! https://pre-commit.ci/#configuration-skip

ci:
    skip: [clang-format]

@joaander
Copy link
Member Author

joaander commented May 4, 2021

@joaander You can skip it! https://pre-commit.ci/#configuration-skip

ci:
    skip: [clang-format]

Excellent find, I'll do that.

@bdice
Copy link
Member

bdice commented May 4, 2021

@joaander With the power of teamwork and enough time, we will read all of the documentation. 😆

@joaander
Copy link
Member Author

joaander commented May 4, 2021

It still seems to try installing the environment :( - https://results.pre-commit.ci/run/github/147663951/1620141512.9i1t5SAyQD6inHgCJ_hhlw

@joaander
Copy link
Member Author

joaander commented May 4, 2021

It still seems to try installing the environment :( - https://results.pre-commit.ci/run/github/147663951/1620141512.9i1t5SAyQD6inHgCJ_hhlw

This is a known and fixed issue: pre-commit-ci/issues#53
pre-commit/pre-commit#1875
Maybe the fix isn't deployed yet?

@bdice
Copy link
Member

bdice commented May 4, 2021

Maybe the fix isn't deployed yet?

Yup. v2.12.1 came out 18 days ago, and that PR was merged 15 days ago.

@joaander joaander merged commit cdcfcd0 into master May 4, 2021
@joaander joaander deleted the pre-commit branch May 4, 2021 16:09
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.

None yet

2 participants