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(pre-commit): Fixes configuration of pre-commit #906

Closed
wants to merge 1 commit into from

Conversation

miigotu
Copy link

@miigotu miigotu commented Sep 26, 2022

Switches autopep8_wrapper to autopep8_mirror, no longer available in pre-commit-hooks directly Switches flak8 to pyca/flake8, no longer available in pre-commit-hooks directly Adds black

Updates hook versions to latest available

Fixes configuration error due to changed schema upstream

[WARNING] normalizing pre-commit configuration to a top-level map.  support for top level list will be removed in a future version.  run: `pre-commit migrate-config` to automatically fix this.
[WARNING] Unexpected key(s) present on https://github.com/pre-commit/pre-commit-hooks: sha

Signed-off-by: miigotu miigotu@gmail.com

Switches autopep8_wrapper to autopep8_mirror, no longer available in pre-commit-hooks directly
Switches flak8 to pyca/flake8,  no longer available in pre-commit-hooks directly
Adds black

Updates hook versions to latest available

Fixes configuration error due to changed schema upstream
```
[WARNING] normalizing pre-commit configuration to a top-level map.  support for top level list will be removed in a future version.  run: `pre-commit migrate-config` to automatically fix this.
[WARNING] Unexpected key(s) present on https://github.com/pre-commit/pre-commit-hooks: sha
```

Signed-off-by: miigotu <miigotu@gmail.com>
@akx akx self-requested a review September 27, 2022 16:24
Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I gave this configuration a shot locally and there're some things that don't quite work for this repo right now.

Comment on lines +44 to +47
- repo: https://github.com/python/black
rev: 22.8.0
hooks:
- id: black
Copy link
Member

Choose a reason for hiding this comment

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

This repo doesn't presently use black for formatting, so please don't add it here.

Comment on lines +29 to +42
additional_dependencies: [
'flake8-bugbear==19.8.0',
'flake8-coding==1.3.2',
'flake8-comprehensions==3.0.1',
'flake8-debugger==3.2.1',
'flake8-deprecated==1.3',
'flake8-docstrings==1.5.0',
'flake8-isort==2.7.0',
'flake8-pep3101==1.2.1',
'flake8-polyfill==1.0.2',
'flake8-print==3.1.4',
'flake8-quotes==2.1.1',
'flake8-string-format==0.2.3',
]
Copy link
Member

Choose a reason for hiding this comment

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

These additional flake8 dependencies could be added later, but they're not currently in use.

Having them in yields some 2000 errors.

Comment on lines +1 to +2
default_language_version:
python: python3.7
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary. We should be able to run pre-commit with other versions of Python. (With this, I get RuntimeError: failed to find interpreter for Builtin discover of python_spec='python3.7'.)

'flake8-debugger==3.2.1',
'flake8-deprecated==1.3',
'flake8-docstrings==1.5.0',
'flake8-isort==2.7.0',
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be something wrong with this version of flake8-isort:

ImportError: cannot import name 'SortImports' from 'isort'

args: ['--django']
- id: check-json
- id: check-yaml
- id: debug-statements
Copy link
Member

Choose a reason for hiding this comment

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

This should also exclude tests/messages/data since there's still (not that it's necessarily relevant) Python 2 code in there.

Comment on lines +20 to +24
- repo: https://github.com/pre-commit/mirrors-autopep8
rev: 'v1.7.0'
hooks:
- id: autopep8
exclude: (docs/conf.py|tests/messages/data/)
Copy link
Member

Choose a reason for hiding this comment

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

autopep8 seems to make changes that conflict with flake8, so I'd suggest adding

[flake8]
max-line-length = 120
ignore = E731,W503,W504

to setup.cfg, which leaves us with a handful (27) manual fixes to do...

'flake8-quotes==2.1.1',
'flake8-string-format==0.2.3',
]
exclude: (docs/conf.py|babel/messages/__init__.py|babel/__init__.py|tests/messages/data|scripts/import_cldr.py)
Copy link
Member

Choose a reason for hiding this comment

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

I think

Suggested change
exclude: (docs/conf.py|babel/messages/__init__.py|babel/__init__.py|tests/messages/data|scripts/import_cldr.py)
exclude: (docs/conf.py|tests/messages/data/)

should be enough, just like for autopep8..?

@miigotu
Copy link
Author

miigotu commented Oct 18, 2022

I'll try and get back in this one and make the requested changes within the next day or so.

@akx
Copy link
Member

akx commented Jan 19, 2023

Closed via #949 (which uses ruff instead).

@akx akx closed this Jan 19, 2023
@miigotu miigotu deleted the pre-commit-update branch January 24, 2023 01:31
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