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

Test run black on self #3114

Merged
merged 8 commits into from Jun 14, 2022
Merged

Test run black on self #3114

merged 8 commits into from Jun 14, 2022

Conversation

saroad2
Copy link
Contributor

@saroad2 saroad2 commented Jun 8, 2022

Description

Removed the hard coded SOURCE list from test_format.py. Now, black is run as part of the Lint CI and looks for sources automatically.

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@felix-hilden felix-hilden added the skip news Pull requests that don't need a changelog entry. label Jun 8, 2022
Copy link
Collaborator

@felix-hilden felix-hilden left a comment

Thanks for submitting! I'm all for removing the hard-coded list 👍

Another option would be to just use black directly in the CI, since pre-commit does the check locally. So having it in Tox would be a bit redundant. Thoughts?

@github-actions
Copy link

github-actions bot commented Jun 8, 2022

diff-shades reports zero changes comparing this PR (ad9c9d9) to main (799adb5).


What is this? | Workflow run | diff-shades documentation

Copy link
Collaborator

@cooperlees cooperlees left a comment

+1 on making test_format dynamically walk src + tests directories (and statically add setup.py) for python source files to test formatting on and continue to do it in parallel test_format tests if possible please. I feel it should be.

# Conflicts:
#	.github/workflows/lint.yml
#	tests/test_format.py
@saroad2
Copy link
Contributor Author

saroad2 commented Jun 11, 2022

Hey @cooperlees ,

Why do you want to run Black on itself as a unit test?

In my opinion, this is more of a system test, checking that Black fits its own standard.

Your suggestion is possible, but I think it's a mistake.

@cooperlees
Copy link
Collaborator

cooperlees commented Jun 11, 2022

Why do you want to run Black on itself as a unit test?

Because we already get the "system test" you're proposing via pre-commit (run pre-commit run -a from the root of the black repo) as @felix-hilden mentioned before my comment. Thus my +1.

The only main advantage I think we run these as a unites is that when they fail, their stack trace might make it easier identify the bug and fix rather than the output that pre-commit running black itself generates.

Copy link
Collaborator

@cooperlees cooperlees left a comment

I also think we don't need a redundant second run of black on itself ...

@saroad2
Copy link
Contributor Author

saroad2 commented Jun 12, 2022

Hey @cooperlees .

How about running black with the --diff flag or --verbose?
This will allow you to see exactly where the files have been changed.

I really think that running Black as a unit test is a mistake.

@cooperlees
Copy link
Collaborator

cooperlees commented Jun 12, 2022

There are benefits to running each way. Minimal, but there are benefits, as mention above.

Let me ask you: What is the benefit you see of running black on itself via Tox and via pre-commit.ci on every PR + commit merge? You have not answered this in every response so far. @felix-hilden and I both don't see the benefit of running black over itself twice. If I'm missing something, please explain that and we'll take it into consideration.

@saroad2
Copy link
Contributor Author

saroad2 commented Jun 12, 2022

Hey @cooperlees ,

The reason I don't think that running pre-commit is enough is that pre-commit run with the latest released Black version, and not the development version. What Tox does is:

  1. Sets a separate virtual environment
  2. Downloads the development version of Black
  3. Runs Black on itself with that version.

This is something that pre-commit doesn't do (unless I'm missing something, and if I do, please let me know!).

I think that the main advantage of running black via the command line on itself is that it's our first opportunity to see how Black works end-to-end. You get few things out of that:

  1. Seeing that Black CLI works
  2. Seeing that Black stands in its own format style
  3. If Black is failed to run on itself, we can see that the error message is meaningful enough so we can understand the error and when it occurred.
  4. If Black source code does not match its own style, we can see that we can understand which line was changed and what changes Black made.

I don't think that we get any added value by running Black as a unit test on each source file. I think that if we run Black end-to-end on itself we can get much more information.

I hope this convinces you. If not, I'll close this PR and create a new one with the change you requested.

I'm sorry if I came a bit too strong, I really want the best for Black and I'm here to help. This project is really important to me.

@cooperlees
Copy link
Collaborator

cooperlees commented Jun 12, 2022

The reason I don't think that running pre-commit is enough is that pre-commit run with the latest released Black version, and not the development version.

This does seem the case after reading docs + testing locally. Due to that I feel we remove pre-commit from running black for black's own CI to make this worth it. My main goal here is keeping black CI as fast as possible to keep it lean and mean + a delight to run for contributors.

  • FWIW - I did break black locally and run pre-commit where it happily formatted ...
  • I also tried to look to see if there was a way to make pre-commit install local commit black - But couldn't work it out with my quick attempts

I personally now don't see the need to run the latest released black in our CI runs. I feel we only want to catch developers breaking black with their commits. Along side that, we introduce new formatting, so I wonder how we've never had the latest released black and local black conflict (I think cause these unit tests didn't error on formatting changes). If no one else disagrees, lets move to tox only running black on itself.

  • I see we also get the benefit here over pre-commit that we will run in all version of python + operating systems we support

Side note

I'm well aware of the value of this kind of test here (I do this for many other projects I maintain as an integration / system test - e.g. bandersnatch). There is no need to explain that. I just thought we already had this and you were duplicating this. It does seems pre-commit does not do what I thought we had here. Sorry if I communicated this poorly, but I thought with @felix-hilden and I both saying it, it was communicated.

@cooperlees cooperlees removed the skip news Pull requests that don't need a changelog entry. label Jun 12, 2022
@cooperlees
Copy link
Collaborator

cooperlees commented Jun 12, 2022

Let's also add a changelog entry here about stopping running via pre-commit and moving to tox to test black on all OS's etc. etc. to summarize the discussion above.

@saroad2
Copy link
Contributor Author

saroad2 commented Jun 13, 2022

Done.
Thanks @cooperlees !

CHANGES.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@cooperlees cooperlees left a comment

@JelleZijlstra What do you prefer here?

CHANGES.md Outdated Show resolved Hide resolved
@cooperlees cooperlees added the skip news Pull requests that don't need a changelog entry. label Jun 14, 2022
Remove non user facing changes.
Copy link
Collaborator

@cooperlees cooperlees left a comment

Thanks for sticking with this, lets just try explain all things and answers peoples main points in responses moving forward and I feel we can down the on PR miscommunication.

@cooperlees cooperlees merged commit 6c1bd08 into psf:main Jun 14, 2022
40 checks passed
@saroad2 saroad2 deleted the test_run_on_self branch Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants