Check for whitespace in CI

[erlend-aasland]

Consider adding a CI check for whitespace errors, a la git diff --check.

I've seen this idea surface a couple of times, most recently by @JelleZijlstra in python/cpython#31695 (comment)

[erlend-aasland]

I remembered I got a PoC branch lying around: https://github.com/erlend-aasland/cpython/tree/add-whitespace-linter

[hugovk]

Sounds good.

Looking at the erlend-aasland/cpython#14 PoC, some ideas:

• Rather than only running on PRs, can it also run for pushes? That way contributors may see and fix a failure on their fork's CI even before opening a PR ("fast feedback loop")

• How about using https://pre-commit.com ? There's a trailing-whitespace hook to trim trailing whitespace.

  • For developers who like pre-commit, it can autofix things for you before you even commit/push

  • For those who don't, there's absolutely no obligation to run locally and the CI will pick things up

  • Can run on pushes and PRs

  • Easily extensible, there are lots of other checks which may be useful (for example, check-yaml to validate CI config)

[erlend-aasland]

Rather than only running on PRs, can it also run for pushes? That way contributors may see and fix a failure on their fork's CI even before opening a PR ("fast feedback loop")

+1

How about using https://pre-commit.com/ ?

Oooh, that sure looks nice!

[hugovk]

Here's an example: hugovk/cpython#22

There's currently a lot of files with trimmable trailing whitespace, see:

• Add pre-commit config and test on CI hugovk/cpython#22 (comment) for a list
• https://github.com/hugovk/cpython/runs/5434180892?check_suite_focus=true for diffs

So we could limit it to certain file types. For example types_or: [c, python]. And/or to certain directories.

[erlend-aasland]

Here's an example: hugovk/cpython#22

Neat! Much more elegant than my PoC 😅

So we could limit it to certain file types. For example types: [c, python]. And/or to certain directories.

+1

[hugovk]

I've updated hugovk/cpython#22 to remove the trailing whitespace, and add:

        types_or: [c, python]
        exclude: ^Lib/test/test_argparse.py$

I excluded that file because a couple of its asserts depend on trailing whitespace:

https://github.com/python/cpython/blob/6927632492cbad86a250aa006c1847e03b03e70b/Lib/test/test_argparse.py#L2179
https://github.com/python/cpython/blob/6927632492cbad86a250aa006c1847e03b03e70b/Lib/test/test_argparse.py#L2196

[arhadthedev]

I excluded that file because a couple of its asserts depend on trailing whitespace

These whitespaces are invisible to a programmer who reads the assertions. Can the following be used instead?

         self.assertEqual(parser.format_help(), textwrap.dedent('''\
             usage: PROG [-h] {}
             main description
             positional arguments:
-              {}          
+              {}          ''' + '''
             options:
               -h, --help  show this help message and exit
         '''))

[ericvsmith]

I excluded that file because a couple of its asserts depend on trailing whitespace

These whitespaces are invisible to a programmer who reads the assertions. Can the following be used instead?

I agree that's better. Also a comment explaining why there are trailing spaces and why the string is constructed that way would help.

[hugovk]

Well turns out there was already a whitespace check run on the CI as part of make patchcheck.

It was broken but has now been fixed: python/cpython#31708

Do we still need the pre-commit suggestion in hugovk/cpython#22 or this issue?

[sweeneyde]

I think it would be nice to have the patchcheck as its own GitHub Action with its own green checkmark, so that sort of common issue gets noticed within seconds, rather than having to wait on the build and the whole test suite to run.

Theoretically, I think we should be able to use actions/setup-python@v3 and not even have to build python at all. Currently, patchcheck uses sys.version to determine what the base branch is, but it could be determined by some sort of patchcheck.py --base_branch ${{ github.base_ref }} feature instead. But I am no wizard at GitHub Actions.

[hugovk]

Please see PR python/cpython#104275.