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.

Sign up for GitHub

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

Jump to bottom

github: use --check instead of --diff for black #318

Merged
merged 2 commits into from
Oct 7, 2020
Merged

github: use --check instead of --diff for black #318

merged 2 commits into from
Oct 7, 2020

Conversation

dlech
Copy link
Collaborator

@dlech dlech commented Oct 4, 2020

--diff does not cause a test to fail, so it is easy to miss

@dlech dlech marked this pull request as draft October 4, 2020 19:02
@dlech
Copy link
Collaborator Author

dlech commented Oct 4, 2020

There are two problems with this change:

  1. It doesn't show the changes needed to fix the failure.
  2. It causes any test after it to fail.

@hbldh
Copy link
Owner

hbldh commented Oct 5, 2020 •
edited
Loading

Yes, I noticed that the diff caused errors on some platforms and not others...

I agree with using --check instead. Code needs to be properly blacked before merging then.

  1. It is irrelevant that the changes needed to fix is not shown, since running black and committing will solve it without the user knowing, right?
  2. I can accept that. Fix code formatting first, commit that and then handle tests. Not that we have many tests though...

@dlech
Copy link
Collaborator Author

dlech commented Oct 5, 2020 

1. It is irrelevant that the changes needed to fix is not shown, since running black and committing will solve it without the user knowing, right?

If it is only one or two lines that need to be changed, it can be faster to manually make the change compared to installing black.

2. I can accept that. Fix code formatting first, commit that and then handle tests. Not that we have many tests though...

I will also look at moving linting to a separate job. Really we only need to lint once instead of for every combination of OS and Python runtime.

@hbldh
Copy link
Owner

hbldh commented Oct 6, 2020

  1. I understand. Will Black be satisfied if the changes are not exactly as it wants them? It is "my way or the highway" kind of code formatter...
    One could catch the exit code of --check and if it is >0 then do a --diff call, print output and then return the exit code?

  2. Yes. It is a bit wasteful of Action minutes.

@dlech dlech force-pushed the dlech-patch-1 branch 4 times, most recently from e634ab0 to cd6d540 Compare October 7, 2020 16:03
dlech and others added 2 commits October 7, 2020 11:06
@dlech
Run black on code
700a32c
@dlech
github: split format/lint check to separate job
4586a16 
Checking the formatting and linting only needs to run once per pull
request. Also, this will allow the other tests to run even if there
is a format or lint error.
@dlech dlech marked this pull request as ready for review October 7, 2020 16:10
@dlech
Copy link
Collaborator Author

dlech commented Oct 7, 2020

This is fixed up now.

@hbldh hbldh merged commit dfa816b into develop Oct 7, 2020
@dlech dlech deleted the dlech-patch-1 branch October 8, 2020 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dlech @hbldh