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 precommit and run precommit on all files #26

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add precommit and run precommit on all files #26

wants to merge 1 commit into from

Conversation

tomelm
Copy link

@tomelm tomelm commented Mar 17, 2017

Adds pre-commit and runs it on all the current files. I skipped a few validations so I'm not making too many big changes and also set the max-line-length to 120:

(venv) flask-graphql|add-precommit ⇒ git commit -am "Add precommit and run precommit on all files"
autopep8 wrapper.........................................................Passed
Check for added large files..............................................Passed
Check python ast.........................................................Passed
Check for byte-order marker..............................................Passed
Check for case conflicts.................................................Passed
Check docstring is first.................................................Passed
Check JSON...........................................(no files to check)Skipped
Check for merge conflicts................................................Passed
Check Yaml...............................................................Passed
Debug Statements (Python)................................................Passed
Fix double quoted strings................................................Passed
Fix End of Files.........................................................Passed
Flake8...................................................................Failed
hookid: flake8

tests/test_graphqlview.py:38:1: E731 do not assign a lambda expression, use a def
tests/test_graphqlview.py:39:1: E731 do not assign a lambda expression, use a def
tests/test_graphqlview.py:452:1: F811 redefinition of unused 'test_supports_pretty_printing' from line 328

Fix python encoding pragma...............................................Passed
Tests should end in _test.py.............................................Failed
hookid: name-tests-test

tests/schema.py does not match pattern ".*_test.py"
tests/test_graphqlview.py does not match pattern ".*_test.py"
tests/test_graphiqlview.py does not match pattern ".*_test.py"
tests/app.py does not match pattern ".*_test.py"

Pretty format JSON...................................(no files to check)Skipped
Fix requirements.txt.....................................................Passed
Trim Trailing Whitespace.................................................Passed
pyupgrade................................................................Passed
Reorder python imports...................................................Passed

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.629% when pulling 93ca54f on tomelm:add-precommit into 724695a on graphql-python:master.

Copy link

@chriskuehl chriskuehl left a comment

Choose a reason for hiding this comment

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

lgtm. often we add pre-commit run --all-files to the tests somewhere which helps to make sure it doesn't regress, but it's up to you if you want to do that or not.

@@ -0,0 +1 @@
pre-commit==0.13.3

Choose a reason for hiding this comment

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

I'd probably call this file requirements-dev.txt since you don't actually need pre-commit to be installed in production (and that's what requirements.txt implies, even if you're not actually doing it in this case).

Choose a reason for hiding this comment

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

Also, we usually don't pin specific versions for our dev dependencies, but that's totally up to you / the project's stance on that.

Copy link
Member

@syrusakbary syrusakbary Mar 18, 2017

Choose a reason for hiding this comment

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

Maybe adding something like the following in the README will actually add more visibility into Yelp's pre-commit project and provide a better understanding to the developer, rather than using a requirements.txt file that might confuse collaborators :)


Collaborate

This project is using pre-commit to help the developer run pre-commit hooks easily, allowing to autoformat Python files, reorder the imports, and other various checks.

You can easily start using it via pip install pre-commit.


Thoughts?

- repo: https://github.com/asottile/reorder_python_imports
sha: v0.3.2
hooks:
- id: reorder-python-imports
Copy link
Member

@syrusakbary syrusakbary Mar 18, 2017

Choose a reason for hiding this comment

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

I've been using isort for checking the import ordering: https://github.com/graphql-python/flask-graphql/blob/master/tox.ini#L30.
Also being used in the automatic linters in graphql-core https://github.com/graphql-python/graphql-core/blob/master/bin/autolinter#L7, and graphene https://github.com/graphql-python/graphene/blob/master/bin/autolinter#L7.

I kind of prefer it over render-python-imports as it seems it does a smarter import grouping, not requiring a import statement per imported var.

If we could use isort instead of render-python-imports would be great! :)

@syrusakbary
Copy link
Member

Other than the comments, everything is looking great... thanks for the work @tomelm! :)

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.

4 participants