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 #1336

Merged
merged 13 commits into from Oct 19, 2022
Merged

Conversation

nikolaik
Copy link
Contributor

@nikolaik nikolaik commented Aug 15, 2022

This adds pre-commit rules (similar to graphene) and enforcement on CI.

@nikolaik nikolaik changed the title 👷 Add pre-commit 👷 Add pre-commit and lint with black and pyupgrade Aug 15, 2022
@nikolaik nikolaik marked this pull request as ready for review August 15, 2022 11:55
@ulgens
Copy link
Collaborator

ulgens commented Aug 15, 2022

Instead of sending a big chunk of changes, can you please create smaller PRs with only 1 or 2 of the pre-commit rules? This is pretty hard to review as is.

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@erikwrede
Copy link
Member

erikwrede commented Aug 15, 2022

@ulgens +1, or just remove the commit that applies the changes and a maintainer runs pre-commit locally to ensure nothing else was changed by accident. Adding black or the other hooks won't introduce breaking changes.

@nikolaik nikolaik force-pushed the chore/pre-commit branch 2 times, most recently from de8b39d to 5d7516f Compare August 15, 2022 17:09
@nikolaik
Copy link
Contributor Author

@ulgens +1, or just remove the commit that applies the changes and a maintainer runs pre-commit locally to ensure nothing else was changed by accident. Adding black or the other hooks won't introduce breaking changes.

Yup, wasn't sure on how to include the actual formatting changes. Reverted the formatting changes so they can be applied by a maintainer instead

@ulgens
Copy link
Collaborator

ulgens commented Aug 15, 2022

@nikolaik @erikwrede If you guys are okay with waiting for 2-3 days, I'll recommend something else.

We should remove Django 2.2 and Python 3.6 from the test matrix, and with their removal, the code should be updated. I can prioritize that cleaning and then we can apply other styling related pre-commit config.

@nikolaik
Copy link
Contributor Author

@nikolaik @erikwrede If you guys are okay with waiting for 2-3 days, I'll recommend something else.

We should remove Django 2.2 and Python 3.6 from the test matrix, and with their removal, the code should be updated. I can prioritize that cleaning and then we can apply other styling related pre-commit config.

Good idea! Something like this #1337 ?

@firaskafri
Copy link
Collaborator

Hello @nikolaik ! Would you please resolve conflicts so we can merge this?

@nikolaik nikolaik changed the title 👷 Add pre-commit and lint with black and pyupgrade 👷 Add pre-commit Sep 23, 2022
@nikolaik
Copy link
Contributor Author

Hello @nikolaik ! Would you please resolve conflicts so we can merge this?

Hi @firaskafri fixed the conflicts now. There are test failures (which are also present on main) and I left the actual formatting up to a maintainer as suggested.

@firaskafri
Copy link
Collaborator

Hello @nikolaik ! Would you please resolve conflicts so we can merge this?

Hi @firaskafri fixed the conflicts now. There are test failures (which are also present on main) and I left the actual formatting up to a maintainer as suggested.

Hi @nikolaik, fixed the previous change that caused tests to fail, but now seems to be some errors with this PR, can you please help?

@nikolaik
Copy link
Contributor Author

Hello @nikolaik ! Would you please resolve conflicts so we can merge this?

Hi @firaskafri fixed the conflicts now. There are test failures (which are also present on main) and I left the actual formatting up to a maintainer as suggested.

Hi @nikolaik, fixed the previous change that caused tests to fail, but now seems to be some errors with this PR, can you please help?

@firaskafri merged with main to fix issues with tests, should be ready to go now!

@firaskafri
Copy link
Collaborator

Hello @nikolaik ! Would you please resolve conflicts so we can merge this?

Hi @firaskafri fixed the conflicts now. There are test failures (which are also present on main) and I left the actual formatting up to a maintainer as suggested.

Hi @nikolaik, fixed the previous change that caused tests to fail, but now seems to be some errors with this PR, can you please help?

@firaskafri merged with main to fix issues with tests, should be ready to go now!

Tests seem to still fail :(

dedent docs/schema.py to allow formatting
@nikolaik
Copy link
Contributor Author

Hi @firaskafri fixed the conflicts now. There are test failures (which are also present on main) and I left the actual formatting up to a maintainer as suggested.

Hi @nikolaik, fixed the previous change that caused tests to fail, but now seems to be some errors with this PR, can you please help?

@firaskafri merged with main to fix issues with tests, should be ready to go now!

Tests seem to still fail :(

Ah, you mean the linter checks. Here you go 81787d9

@firaskafri firaskafri self-requested a review September 25, 2022 22:23
@ulgens ulgens mentioned this pull request Sep 26, 2022
@firaskafri firaskafri self-requested a review October 18, 2022 13:29
@firaskafri firaskafri merged commit 4517e32 into graphql-python:main Oct 19, 2022
superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request Jul 19, 2023
* 🔧 Add pre-commit config

Similar to graphene and graphene-sqlalchemy

* ⬆ Bump black

* 👷 Lint on CI

* ⬆ Bump flake8-black

* 🔧 Keep excluding migrations

* ⬆ Bump flake8

* 🔧 Remove black and flake8 from tox config

* ⬆ Update pre-commit versions

* Upgrade syntax to python 3.7+

* Format with pre-commit

dedent docs/schema.py to allow formatting

* Fix tests on python 3.7
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.

None yet

4 participants