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

ci: add whitespace check #1881

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Mar 6, 2024

This adds a whitespace check to the CI that fails if there is trailing white space or a file does not end with a newline.

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch 8 times, most recently from a241ac5 to 7648faa Compare March 6, 2024 18:03
@jdchristensen
Copy link
Collaborator

I thought it was pretty funny that in an earlier version, whitespace.yml didn't end in a newline... 😄

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch 3 times, most recently from 67fe2db to b0c0bef Compare March 6, 2024 18:08
@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch 6 times, most recently from f7f317f to de57fac Compare March 6, 2024 18:26
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

@jdchristensen OK sorry for the noise. I think I'm done with this sprint. The first commit adds the CI checks. The second fixes the end newlines and the third fixes the trailing white spaces. Just to be sure I will push each commit separately again.

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch from de57fac to 062a2df Compare March 6, 2024 18:28
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

Here is the first commit, it should fail the check as there are trailing whitespaces in the library. And it does.

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch 2 times, most recently from e9931e4 to 60696f5 Compare March 6, 2024 18:30
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

Here is the second commit, it should fail because only the trailing white spaces have been fixed but there are still problems with the end newlines.

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch from 60696f5 to 13cc131 Compare March 6, 2024 18:30
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

Here is the third commit. The end newlines have now been fixed and all the checks pass.

@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

Once we merge this, I'll prepare a PR that ignores these commits from git blame so that there aren't spurious blames everywhere and our history is clean. I can't do that now as the hash might change.

@Alizter Alizter requested a review from jdchristensen March 6, 2024 18:36
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

Oh and I forgot to drop the commit removing the whitespace, here is that done now.

@jdchristensen
Copy link
Collaborator

@Alizter, thanks for figuring out how to do this. Is it easy to do something like git diff --check master...HEAD to do the check on the entire diff? (Maybe master...HEAD would need to be replaced with foo HEAD, where foo is the starting commit of the PR? Not sure the context this is run in.)

@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

@jdchristensen We have a shallow clone of the commits in the PR when doing the whitespace checks. I'll see if I can make it a single check rather than iterating over each commit.

Hmm actually this depends on whatever git log outputs rather than going over each commit. Let me ponder on it some more.

@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch 9 times, most recently from a174fa1 to 78a5ba2 Compare March 6, 2024 23:14
Alizter added 15 commits March 7, 2024 00:19
This adds a whitespace check to the CI that fails if there is trailing
white space or a file does not end with a newline.

Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: f0c063fd-98da-45ac-8ad8-a79a093f737b -->
<!-- ps-id: 925b1ad7-6f77-4af7-9bcc-67adb5a6f2f9 -->

Signed-off-by: Ali Caglayan <[email protected]>
<!-- ps-id: 1af6c3bc-7c17-40af-b082-6c263579d996 -->

Signed-off-by: Ali Caglayan <[email protected]>
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: cc54f85f-455b-454f-91a6-069e3be58f99 -->
<!-- ps-id: 27aaca6c-4c08-4edc-86c4-576f379161ef -->

Signed-off-by: Ali Caglayan <[email protected]>
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: f20d7036-af32-4dc7-8497-a4fc436e560e -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: 070a142f-9cf5-4141-a1f0-02b2504cd70d -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: 895e000d-e169-4a6b-a2d4-e2a3a25dde57 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: 5a7ce2f1-dfb6-4426-99e5-8ba60f468934 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: ae1e2e79-392a-47d3-906b-8d45142cd5f1 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: c846bb6b-9189-4f94-a9cd-1c352e7866e9 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: b4894953-798e-44fe-98a4-e4d6772f7617 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: 56342511-dc5b-49c6-87ac-08def4397d93 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: 3d7515f0-da9f-487c-a21b-c7d543f84d67 -->
Signed-off-by: Ali Caglayan <[email protected]>

<!-- ps-id: fb46e55d-ab1f-4436-b100-38c54d80e13f -->
@Alizter Alizter force-pushed the ps/rr/ci__add_whitespace_check branch from 78a5ba2 to dee36b9 Compare March 6, 2024 23:19
@Alizter
Copy link
Collaborator Author

Alizter commented Mar 6, 2024

I got something sort-of working, but I can't get github actions to print the commit, file and location of the whitespace. I am out of energy for today, so I will take another look later.

@Alizter Alizter marked this pull request as draft March 6, 2024 23:22
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