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 autopep8 git hook #1100

Merged
merged 8 commits into from
Aug 6, 2021
Merged

Conversation

brryan
Copy link
Contributor

@brryan brryan commented Aug 6, 2021

Background

  • flake8 is effective for enforcing python syntax, but it doesn't apply inline updates.
  • autopep8 does provide inline fixes as an option

Purpose of Pull Request

  • Add autopep8 git hook to apply inline formatting fixes to modified python code prior to calling flake8.
  • Fixes re-git issue #18

Description of changes

  • Add pre-commit-autopep8 script and update pre-commit and install-hooks.sh to Draco/environment/git.

Status

@brryan brryan requested a review from KineticTheory August 6, 2021 16:36
@brryan brryan self-assigned this Aug 6, 2021
@brryan
Copy link
Contributor Author

brryan commented Aug 6, 2021

@KineticTheory Here you go. I tested this with some simple garbled formatting in a python file and it worked for me as a commit hook.

I'm worried that at some point autopep8 will make a change that flake8 flags as an error, which will make committing with active hooks impossible to get past the CI... These two formatters should be following the same standards, but I've already encountered scenarios where autopep8 doesn't fix things that flake8 complains about. Maybe their options can be exactly synchronized? Another option would be to run flake8 on each file and record the errors, then run autopep8 to make changes, and then run flake8 again, checking that no new errors were introduced by autopep8. A bit involved...

@@ -13,7 +13,7 @@
###########################################################
# CONFIGURATION:
# select which pre-commit hooks are going to be installed
HOOKS="pre-commit pre-commit-clang-format pre-commit-flake8 pre-commit-fprettify"
HOOKS="pre-commit pre-commit-clang-format pre-commit-autopep8 pre-commit-flake8 pre-commit-fprettify"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need pre-commit-flake8 if we adopt pre-commit-autopep8?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't mind switching to autopep8 from flake8, that would solve the above consistency issue I brought up. I do like that autopep8 will automatically apply formatting changes, but I don't know if there are reasons for us to still prefer flake8 at the CI level

Copy link
Collaborator

Choose a reason for hiding this comment

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

The superlinter CI check that we use will run flake8. I want to keep that CI because it has a very low maintenance cost. It sounds like we need to keep both for now.

@KineticTheory KineticTheory merged commit a659254 into lanl:develop Aug 6, 2021
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