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

Specify black version in pre-commit config #970

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Nov 12, 2020

This isn't a perfect solution to #909, because I think I should probably change the github action to match, but it should resolve issues for folk using the pre-commit hook and getting conflicts with what's in Github Actions for now.

@imsahil007
Copy link
Contributor

We will have to specify black version 20.8b1 everywhere in the documentation. Basically the contribution section as well as the requirements.txt file.
One other option can be to leave it as it used to be. And update instruction for install black from pip install black to pip install git+git://github.com/psf/black
We won't have to update the versions every time we want to use any new version of black.

@imsahil007
Copy link
Contributor

We will have to specify black version 20.8b1 everywhere in the documentation. Basically the contribution section as well as the requirements.txt file.
One other option can be to leave it as it used to be. And update instruction for install black from pip install black to pip install git+git://github.com/psf/black
We won't have to update the versions every time we want to use any new version of black.

Updating requirements.txt content from black to git+git://github.com/psf/black works as well.

@terriko
Copy link
Contributor Author

terriko commented Nov 12, 2020

Yeah, this is a temporary solution at best. I'm not sure I like it. Did you want to take a stab at something more comprehensive? I can close this one and leave you to it if you'd like!

I think what we want is to ensure we're getting the latest from pip install black in both cases, I just didn't see a way to fix that in the github actions hook we're using right now and needed a temporary fix for today.

@terriko
Copy link
Contributor Author

terriko commented Nov 12, 2020

(I can just diable my conflicting pre-commit for today.)

@terriko terriko closed this Nov 12, 2020
@terriko terriko reopened this Dec 9, 2020
@terriko terriko merged commit 1d9157b into intel:master Dec 9, 2020
@Molkree
Copy link
Contributor

Molkree commented Apr 27, 2021

@terriko @imsahil007
So I was looking at why you have pinned versions in pre-commit-config and stumbled on this PR (and #973). Few thoughts:

it seemed like I had 20.8b1 (latest from pypi) installed, but it seemed like CI was using something else

That's because at that point in time you had stable rev specified in the pre-commit-config. One might think that it's actually a setting it accepts (like "please, pull only release tags, not latest commit or pre-release tags") but pre-commit doesn't provide such functionality, it only accepts immutable refs (per docs). And yeah stable is actually a tag in Black repository! And it only was moved to point at 20.8b1 release 20 days ago (they probably just forgot) while GitHub Action you use in CI moved to that release in September. That is why you were getting different versions back then.

They just released a new version yesterday and stable still points at the old release, so it's not reliable I think.

I'd still really like a solution that told pre-commit to use the latest release instead of a specific tag, but as best I can tell it doesn't support that at this time.

As I noted above:

pre-commit configuration ... intentionally doesn't provide facilities for "unpinned latest version" for hook repositories.


Now how to sync pre-commit and CI and get the latest tagged version?
First of all, I'm not sure separate 3rd party Action for Black is needed (you use jpetrucciani/black-check@master now), it's just another layer that adds delays.
Black provides a simple example of usage in GH workflow.

But if we want to sync pre-commit and CI maybe just run pre-commit in CI? pre-commit run [hook-id] --all-files can replace usage of Actions now and passing hook-id will still allow separating Black and isort in different jobs.

Also can add a new scheduled workflow that will check for updated hooks in pre-commit-config and open PR if needed.
pre-commit autoupdate will update config with latest tagged versions, then just use something like peter-evans/create-pull-request. Schedule it to run once a week and we'll have almost latest tags all the time synced with CI.

Of course it also applies to isort.

@Molkree
Copy link
Contributor

Molkree commented May 15, 2021

and stable still points at the old release, so it's not reliable I think

Well, looks like they are committed to updating it more often now.

Black provides a stable tag for people who want to move along as Black developers deem the newest version reliable. Here the Black developers will move once the release has been problem free for at least ~24 hours from release.

It's fine to use it in CI but in pre-commit it's still discouraged (because it's mutable rev).

Maybe use pre-commit.ci? It will autoupdate pre-commit config weekly and run checks on/format all PRs automatically. It's free for open source repos but not free for "organization" repositories whatever that means. Not sure if cve-bin-tool is former or the latter. And I guess it might be problematic legally?

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.

3 participants