-
Notifications
You must be signed in to change notification settings - Fork 768
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
Miscellaneous CI fixes #1447
Conversation
There was a problem hiding this 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 |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
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.
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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
)
And thanks again for making these improvements! |
setup.py
Outdated
"black", | ||
"ruff", |
There was a problem hiding this comment.
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
Great work @kiendang ! Is it ready to be merged? |
Yup I think so. @sjdemartini? Thanks for the reviews btw! |
* 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
Makefile
following the switch to rufftox.ini
fixesruff
version inpre-commit
. Previously runningruff
directly yields different result compared topre-commit
. Updatingruff
fixed that.