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

Add whitespace check to CI #8316

Closed
andreasabel opened this issue Jul 25, 2022 · 3 comments · Fixed by #8320
Closed

Add whitespace check to CI #8316

andreasabel opened this issue Jul 25, 2022 · 3 comments · Fixed by #8320
Assignees
Labels
continuous-integration newcomer re: code quality Internal issue: concerning code quality / refactorings

Comments

@andreasabel
Copy link
Member

Lifted from #6370 (comment) :
I think whitespace checks should be part of the CI, see e.g. https://github.com/agda/agda/blob/3044510c8966a1876951e807f1393bbc3b5e6169/src/github/workflows/whitespace.yml
(configuration at https://github.com/agda/agda/blob/3044510c8966a1876951e807f1393bbc3b5e6169/fix-whitespace.yaml).

The cited workflow from the Agda repo can be copied over, with some modification to the fix-whitespace.yaml configuration file.

@chshersh
Copy link
Member

I would like to highlight the importance of this issue (an #6370 as well) by sharing my anecdotal story about contributing to cabal.

Several years ago I've submitted a patch to Cabal and part of the changes were whitespaces cleanups in a couple of lines. Trailing whitespaces were automatically removed by my editor and I haven't figured how to disable this automatic cleanup.

I thought that a few lines is not a big deal. But to my surprise, PR reviewers told me that next time it's better to put all such cleanups in separate commits. It felt extremely off-putting to me and I immediately stopped contributing to Cabal after that. I'm not going to spend my free time configuring my editor to disable whitespace cleanup just for a single codebase (especially when I don't know how to do this) and I'm definitely not going to do extra work just to put a few trailing whitespaces removals in separate commits.


This is just a single example but I might not be the only one who had similar experiences. If we cleanup all whitespaces in Cabal and put this check on CI, we remove the possibility of having such conversation in the first place. On top of that, if all commits are going to be "Squashed and Merged" eventually, it doesn't make sense to spend time putting them into separate commits.

@philderbeast
Copy link
Collaborator

philderbeast commented Jul 25, 2022

I've been contributing some hlint fixes to liquid haskell and liquid fixpoint projects and sometimes using HLS in vscode to apply the fix. This can result in lots of whitespace trimming at the end of lines. Tooling including the github web one for review of code changes in a pull request can hide these whitespace changes. Here's a small example of what can happen:

https://github.com/ucsd-progsys/liquid-fixpoint/pull/580/files

@andreasabel
Copy link
Member Author

Yes, we (at least I) don't want to review PRs where whitespace changes are sprinkled all over. It is best practice to place these into a separate commit. Esp. you don't want to get the confusion of merge/rebase conflicts because of whitespace issues. If they are in a separate commit they are easier to handle.
However, it is also best practice to keep the repo free of whitespace issues all the time. A tool such as https://github.com/agda/fix-whitespace can help here.

@andreasabel andreasabel added the re: code quality Internal issue: concerning code quality / refactorings label Jul 28, 2022
mergify bot pushed a commit that referenced this issue Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
continuous-integration newcomer re: code quality Internal issue: concerning code quality / refactorings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants