GeezerVison is showing github.com — Check for whitespace in CI · Issue #429 · python/core-workflow
Check for whitespace in CI #429DescriptionReacted by Jelle Zijlstra, Alex Waygood and Hugo van Kemenade Activityerlend-aasland commented on Mar 5, 2022 hugovk commented on Mar 5, 2022 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)
Reacted by Alex Waygood, Erlend E. Aasland and Jonas Kittner erlend-aasland commented on Mar 5, 2022
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 commented on Mar 5, 2022 Here's an example: hugovk/cpython#22
There's currently a lot of files with trimmable trailing whitespace, see:
So we could limit it to certain file types. For example types_or: [c, python]. And/or to certain directories. Reacted by Erlend E. Aasland erlend-aasland commented on Mar 5, 2022
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 commented on Mar 5, 2022 arhadthedev commented on Mar 5, 2022
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 commented on Mar 6, 2022
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. Reacted by Erlend E. Aasland, Oleg Iarygin, Hugo van Kemenade and Ezio Melotti hugovk commented on Mar 14, 2022 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 commented on Apr 17, 2022 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. Reacted by Alex Waygood, Erlend E. Aasland, Hugo van Kemenade and Ezio Melotti hugovk commented on May 7, 2023 Reacted by Itamar Oren and Erlend E. Aasland erlend-aasland commented on Jun 27, 2023
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|