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 (take 2) #3999

Closed
wants to merge 12 commits into from
4 changes: 4 additions & 0 deletions .flake8
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[flake8]
max-line-length = 79
select = B,C,E,F,W,T4,B9,B950
ignore = E402, W503, W504
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

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

This overrides existing flake8 config present in tox.ini.

Copy link
Member Author

@hjoliver hjoliver Dec 16, 2020

Choose a reason for hiding this comment

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

Yes, that's why the fast tests were failing on the original branch, until I added our usual warning ignore settings here instead of the ones @sgaist used.

As per my comments above, pycodestyle (and flake8, which wraps pycodestyle) ignores the tox.ini config on one of my dev platforms, because of inline comments in the file, even though the pycodestyle version claims to be the same.

However, now that I understand the cause I could move those comments, and we could drop this new config. Is there any reason at all to have both?

Annoyingly, it seems tox.ini needs duplicate pycodestyle settings, under [pycodestyle] and [flake8] sections, unless you never run pycodestyle alone. Also, flake8 ignores W503 and W504 by default (but not if you explicitly ignore anything else).

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bit of complexity with all the possible configuration files that can be used. There's also the pyproject that might be used but may not be supported by all the tools.

I have a project which has flake8 in tox.ini, bandit in its own file because they were (might still) not supporting the pyproject.toml nor tox.ini, and the rest is in pyproject.toml. I like the idea of having a maximum of configurations in a single file but that is rather up to the taste of the maintainer(s).

1 change: 0 additions & 1 deletion .github/workflows/test_fast.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ jobs:

- name: Style
run: |
pycodestyle
flake8
etc/bin/shellchecker

Expand Down
7 changes: 7 additions & 0 deletions .isort.cfg
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[settings]
multi_line_output=3
include_trailing_comma=true
force_grid_wrap=0
use_parentheses=true
ensure_newline_before_comments=true
line_length=79
33 changes: 33 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
- repo: https://github.com/timothycrosley/isort
rev: 5.6.4
hooks:
- id: isort
files: .*.py
- repo: https://github.com/psf/black
rev: 20.8b1
hooks:
- id: black
args: ["--line-length", "79", "-t", "py37"]
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v3.2.0
hooks:
- id: check-ast
Copy link
Member

Choose a reason for hiding this comment

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

Don't think we need this (slow) one if we are using black in its regular mode (it parses into ast anyway).

Also I think pytest would bail anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds convincing. Would you agree @sgaist ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, there might also be other hooks that also uses the AST for their work, so it's one that can be dropped in this case.

- id: check-case-conflict
- id: trailing-whitespace
hjoliver marked this conversation as resolved.
Show resolved Hide resolved
- id: end-of-file-fixer
hjoliver marked this conversation as resolved.
Show resolved Hide resolved
- id: debug-statements
- id: check-added-large-files
- id: check-docstring-first
- id: check-yaml
exclude: conda/meta.yaml
- repo: https://github.com/PyCQA/flake8
rev: 3.8.4
hooks:
- id: flake8
oliver-sanders marked this conversation as resolved.
Show resolved Hide resolved
- repo: https://github.com/PyCQA/bandit
rev: 1.6.2
hooks:
- id: bandit
7 changes: 7 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,13 @@ add their details to the [Code Contributors](#code-contributors) section of
this file as part of their first Pull Request, and reviewers are responsible
for checking this before merging the new branch into *master*.

To start modifying cylc-flow and test your changes, first call
`pip install -e .[all]` to install the package in development mode along with
all dependencies. Then call `pre-commit install` so that the git pre-commit
hooks will run on commit. [pre-commit ](https://pre-commit.com) is used for
code style consistency. It can also be run manually with the `pre-commit run`
command (use --files to run on a reduced set of files).

## Code Contributors

The following people have contributed to this code under the terms of
Expand Down
3 changes: 3 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ def find_version(*file_paths):
'main_loop-log_memory': [
'pympler',
'matplotlib'
],
"dev": [
"pre-commit"
]
}
extra_requires['all'] = (
Expand Down
1 change: 1 addition & 0 deletions tests/functional/jinja2/filters/Jinja2Filters/truly.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,6 @@
# You should have received a copy of the GNU General Public License
# along with this program. If not, see <http://www.gnu.org/licenses/>.


def truly(bar):
return 'true'
6 changes: 4 additions & 2 deletions tests/functional/validate/68-trailing_whitespace.t
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ cat >'flow.cylc' <<'__FLOW_CONFIG__'
final cycle point = 20010101T18
[[graph]]
T00 = """
# NOTE: don't let editor strip trailing space on next line
foo | bar \
# pre-commit hook removes trailing space so add it with sed below
foo | bar BACKSLASH_TRAILING_WHITESPACE
=> baz & qux
pub
"""
Expand All @@ -38,6 +38,8 @@ cat >'flow.cylc' <<'__FLOW_CONFIG__'
"""
__FLOW_CONFIG__

sed -i 's/BACKSLASH_TRAILING_WHITESPACE/\\ /' 'flow.cylc'

run_fail "${TEST_NAME_BASE}-simple-fail" cylc validate 'flow.cylc'
cmp_ok "${TEST_NAME_BASE}-simple-fail.stderr" <<'__ERR__'
FileParseError: Syntax error line 7: Whitespace after the line continuation character (\).
Expand Down
5 changes: 2 additions & 3 deletions tox.ini
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
[pycodestyle]
; E402 module level import not at top of file
; W503/4 line break before/after binary operator
ignore=
; module level import not at top of file
E402,
; line break before binary operator
W503,
; line break after binary operator
W504
exclude=
.git,
Expand Down