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 code linting #221

Merged
merged 12 commits into from
Dec 21, 2024
Merged

Conversation

altheaden
Copy link
Collaborator

This PR adds pre-commit for automatic code linting. I've also added pre-commit to the CI build workflow to automatically lint any incoming PRs.

I'm including changes that will satisfy all pre-commit hooks except flake8. The changes flake8 wants require manual editing to many code files, so we're opting to only deal with those issues on a file-by-file basis as new changes come in. Because of this, the linting step of CI for this PR will fail, but that's expected.

Finally, I'm including a small bugfix to the docs CI workflow.

@altheaden altheaden added clean up ci Changes that affect github actions CI labels Dec 20, 2024
rev: 5.13.2
hooks:
- id: isort
exclude: "geometric_features/__init__.py"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@xylar isort reordering the imports for this file broke everything, so I've excluded it here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's follow up on that in a separate PR. We don't need all the __future__ stuff anymore. That was for python 2. Maybe that will help. But if that's not the problem, we maybe need to add missing imports somewhere else.

@altheaden altheaden requested a review from xylar December 20, 2024 20:22
Comment on lines 1931 to 1924
],
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's exclude this file from pre-commit checks because I just added a test in #220 to make sure it's formatted as expected and the two might slightly disagree. In particular, the json output seems to not put the desired newline at the end of the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I went ahead and took care of this in order to get this in. I hope that's okay.

Copy link
Collaborator

@xylar xylar left a comment

Choose a reason for hiding this comment

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

Update: I see you added pre-commit. I've added flake8 andisort here as well:
https://github.com/MPAS-Dev/geometric_features/blob/main/dev-spec.txt#L14-L17

.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@xylar
Copy link
Collaborator

xylar commented Dec 21, 2024

We're purposefully ignoring the flake8 failures here for now. So I will merge without CI passing for now.

@xylar xylar merged commit 2183682 into MPAS-Dev:main Dec 21, 2024
5 of 6 checks passed
@xylar
Copy link
Collaborator

xylar commented Dec 21, 2024

Thanks @altheaden!!

@altheaden altheaden deleted the add-pre-commit branch December 23, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Changes that affect github actions CI clean up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants