Code style: format the code using darker
#8875
Conversation
tox.ini
Outdated
| deps = | ||
| pre-commit | ||
| commands = | ||
| pre-commit run |
There was a problem hiding this comment.
I'm in doubt here about how darker will find the diff to check since there won't be files to check at this point because everything is already committed.
We may need to specify PRE_COMMIT_FROM_REF (to the base branch) and PRE_COMMIT_TO_REF (to latest commit of this branch) to work around this.
See https://github.com/akaihola/darker#customizing-darker-black-and-isort-behavior
There was a problem hiding this comment.
Somehow, we have to pass github.event.pull_request.base.sha and github.event.pull_request.head.sha to CircleCI to pass this values to darker as pre-commit environment variables.
There was a problem hiding this comment.
CircleCI built in env vars might give you at least one of those. CIRCLE_SHA1 is available, but the base sha might not be
There was a problem hiding this comment.
Yeah. Unfortunately, it seems that CircleCI knows nothing about these GitHub variables and doesn't expose them to us.
I don't think it's a big problem, tho. However, when using a different "base branch" on GitHub, this check won't be accurate. I'd say that's fine for now and we can tweak it as we go when we find a solution for that case. We could use GitHub Actions for this, but I don't think it we want to setup something like this outside CircleCI for now.
So, I'm happy to move forward with this PR and start giving it a try.
We have been talking about doing incremental formatting of our code. Since `black` does not support this, we are using `darker` that support this, calls `black` + `isort` behind the scenes and integrates well with pre-commit.
humitos
force-pushed
the
humitos/setup-linter
branch
from
1480eee to
b07be6e
Compare
|
@readthedocs/backend is everybody happy with this PR? should I merge it? |
| commands = | ||
| # FIXME: use `github.event.pull_request.base.sha` in `--from-ref` because if | ||
| # the base branch is different, this won't work as expected | ||
| pre-commit run --from-ref master --to-ref HEAD |
There was a problem hiding this comment.
Does this change the files as well? Or do we need to run black/pre-commit by ourselves?
There was a problem hiding this comment.
This does change the files locally, but it does require a commit first -- this doesn't use staged files.
There was a problem hiding this comment.
I commented in the common PR as well. It does work on staged files to me. I'm not sure why it doesn't for you or if you have it configured in a different way.
|
I was able to actually test pre-commit, and found the workflow a little confusing. I'm okay merging this and getting folks testing this, but I think it needs documentation. Previously, it was possible to run pre-commit on staged changes, and that isn't the case with this configuration. If there is an invocation of pre-commit that allows darker to be used on staged files, we should document and also use that, alongside the usage set up here already. For reference, my notes are here: readthedocs/common#102 (comment) |
|
Just as another note. I'm 👍 on this, but I'm also 👍 on a huge PR that lints the whole codebase. I think that's going to be the least pain in the long run, and allow us to get to a simpler setup locally & in CI. |
|
I think we have a good plan for now, I'd say lets continue with the plan we discussed. Linting new changes as we author them is where we get the value from linting. Linting old code doesn't provide much value. The plan we have now should plug into CI fine. We do probably need to do a bit more to make linting locally, and fixing lint issues locally, easier. I'm describing some docs on how we're expected to use pre-commit in our normal workflows, I couldn't find where we even documented pre-commit usage previously. |
|
I'm trying to move forward with this. I'll make the changes required to merge it when tests pass. |
We have been talking about doing incremental formatting of our code. Since
blackdoes not support this, we are usingdarkerthat support this, callsblack+isortbehind the scenes and integrates well with pre-commit.A new check is added at CircleCI level that will enforce the PRs to be blacked (only the diff, tho).