graphene-django icon indicating copy to clipboard operation
graphene-django copied to clipboard

👷 Add pre-commit and lint with black and pyupgrade

Open nikolaik opened this issue 2 years ago • 5 comments

This adds some basic pre-commit rules and enforcement on CI.

Black gets a version bump and files are formatted.

Pyupgrade is run to upgrade pre 3.6 syntax.

nikolaik avatar Aug 15 '22 10:08 nikolaik

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.

ulgens avatar Aug 15 '22 13:08 ulgens

@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.

erikwrede avatar Aug 15 '22 13:08 erikwrede

@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

nikolaik avatar Aug 15 '22 17:08 nikolaik

@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.

ulgens avatar Aug 15 '22 17:08 ulgens

@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 https://github.com/graphql-python/graphene-django/pull/1337 ?

nikolaik avatar Aug 15 '22 17:08 nikolaik

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

firaskafri avatar Sep 23 '22 08:09 firaskafri

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.

nikolaik avatar Sep 23 '22 09:09 nikolaik

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 avatar Sep 24 '22 12:09 firaskafri

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!

nikolaik avatar Sep 25 '22 15:09 nikolaik

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 :(

firaskafri avatar Sep 25 '22 19:09 firaskafri

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

nikolaik avatar Sep 25 '22 20:09 nikolaik