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

Fix #8316: run fix-whitespace --check as CI workflow #8320

Merged
merged 3 commits into from
Jul 28, 2022
Merged

Conversation

andreasabel
Copy link
Member

Fix #8316: run fix-whitespace --check as CI workflow.

Also: fix existing whitespace violations (separate commit).

The whitespace checker can be configured in fix-whitespace.yaml.

@Mikolaj
Copy link
Member

Mikolaj commented Jul 25, 2022

Would it make sense to add a line to CONTRIBUTING explaining how to run the check locally?

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm modulo minor comments. Thank you!

@andreasabel
Copy link
Member Author

andreasabel commented Jul 26, 2022

Weird, how come my latest push of workflow whitespace.yml checked out latest master and thus found a whitespace violation that wasn't on the branch of this PR??
https://github.com/haskell/cabal/runs/7515526901?check_suite_focus=true#step:7:7

Run $HOME/.local/bin/fix-whitespace --check
[ Violation detected ] release-notes/Cabal-3.8.1.0.md
Error: Process completed with exit code 1.

The violation is a "No newline at end of file" at

- Previously, for unknown new versions of GHC, it was not filtering GHC arguments at all, while now it filters them in the same way as for the last known GHC version. This seems a better default and it's one less place to update at release time. Perhaps erroring out or emitting a warning would be safer, but we already emit a general warning elsewhere.

I am using the vanilla actions/checkout: https://github.com/haskell/cabal/pull/8320/files#diff-0ea7b088f801fe875e2de46ed9180c6e04bb6f0bd9e7311428786195831e6b90R19

@andreasabel
Copy link
Member Author

Would it make sense to add a line to CONTRIBUTING explaining how to run the check locally?

Just pushed a commit to that extend.

@andreasabel andreasabel marked this pull request as ready for review July 26, 2022 12:03
@Mikolaj
Copy link
Member

Mikolaj commented Jul 26, 2022

Would it make sense to add a line to CONTRIBUTING explaining how to run the check locally?

Just pushed a commit to that extend.

Looks fine, thank you.

Copy link
Member

@chshersh chshersh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix-whitespace tool is cool, I might want to use it in my own projects as well 🙂

andreasabel added a commit that referenced this pull request Jul 27, 2022
@andreasabel andreasabel added the merge me Tell Mergify Bot to merge label Jul 28, 2022
@andreasabel
Copy link
Member Author

I reorganized the 6 commits into 3, but these 3 are independent and should be kept separate.

Why didn't label "merge me" do anything in 4 hours?

@ulysses4ever
Copy link
Collaborator

Because of #8285.

@andreasabel andreasabel merged commit c21dbcd into master Jul 28, 2022
@andreasabel andreasabel deleted the whitespace branch July 28, 2022 11:49
@andreasabel
Copy link
Member Author

Sorry, I skipped the 2-day waiting period because this PR would rot too fast: it would get broken every time someone introduces a whitespace violation into the code base.

@andreasabel andreasabel self-assigned this Jul 28, 2022
@andreasabel andreasabel added the re: code quality Internal issue: concerning code quality / refactorings label Jul 28, 2022
@ulysses4ever
Copy link
Collaborator

tldr; We need to backport this to 3.8 if we want a working CI on 3.8

Reasoning: see #8503 and #8505. They are required for working CI since ghcup updated the recommended GHC -- no matter the branch. And #8505 mentions the whitespace job, so it couldn't be (easily) backported without this one being backported first.

@ulysses4ever
Copy link
Collaborator

@mergify backport 3.8

@mergify
Copy link
Contributor

mergify bot commented Oct 7, 2022

backport 3.8

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Oct 7, 2022
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge re: code quality Internal issue: concerning code quality / refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add whitespace check to CI
5 participants