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

Miscellaneous CI fixes #1447

Merged
merged 8 commits into from Aug 9, 2023
Merged

Miscellaneous CI fixes #1447

merged 8 commits into from Aug 9, 2023

Conversation

kiendang
Copy link
Collaborator

@kiendang kiendang commented Aug 9, 2023

  • Fix Makefile following the switch to ruff
  • Some miscellaneous tox.ini fixes
  • Updating ruff version in pre-commit. Previously running ruff directly yields different result compared to pre-commit. Updating ruff fixed that.

@firaskafri firaskafri self-requested a review August 9, 2023 16:34
Copy link
Collaborator

@sjdemartini sjdemartini left a comment

Choose a reason for hiding this comment

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

Overall looks good! A couple quick suggestions

Makefile Outdated

.PHONY: format ## Format code
format:
black graphene_django examples setup.py
tox -e pre-commit
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this actually has a different effect, since it won't apply to all existing files and will just apply to uncommitted changes. Perhaps this should be pre-commit run --all-files instead of just pre-commit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah this runs the pre-commit tox env, not the pre-commit command. In tox.ini you will see

graphene-django/tox.ini

Lines 37 to 41 in 7bd4a68

[testenv:pre-commit]
skip_install = true
deps = pre-commit
commands =
pre-commit run {posargs:--all-files --show-diff-on-failure}

that's the command being run

but I agree on adding back black and reversing this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah gotcha, okay 👍

@@ -10,15 +10,15 @@ dev-setup:

.PHONY: tests ## Run unit tests
tests:
py.test graphene_django --cov=graphene_django -vv
pytest graphene_django --cov=graphene_django -vv

.PHONY: format ## Format code
format:
Copy link
Collaborator

Choose a reason for hiding this comment

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

side suggestion here: perhaps we add back black in the dev_requires dependencies in setup.py (so dev-setup in this Makefile also installs black, since that will likely be useful for folks' editor configurations just like ruff)

@sjdemartini
Copy link
Collaborator

And thanks again for making these improvements!

setup.py Outdated
Comment on lines 29 to 30
"black",
"ruff",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should presumably pin these to the same versions as in pre-commit so we ensure the same formatting

@firaskafri
Copy link
Collaborator

Great work @kiendang ! Is it ready to be merged?

@kiendang
Copy link
Collaborator Author

kiendang commented Aug 9, 2023

Yup I think so. @sjdemartini? Thanks for the reviews btw!

@firaskafri firaskafri merged commit 79b4a23 into main Aug 9, 2023
12 checks passed
@firaskafri firaskafri deleted the ci-misc branch August 9, 2023 17:48
superlevure pushed a commit to loft-orbital/graphene-django that referenced this pull request Aug 10, 2023
* Update Makefile

* django master requires at least python 3.10 now

* Allow customizing options passed to tox -e pre-commit

* py.test -> pytest

* Update ruff

* Fix E721

Do not compare types, use `isinstance()`

* Add back black to dev dependencies

* Pin black and ruff versions
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

3 participants