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

GitHub Action to spellcheck and lint Python code #333

Merged
merged 6 commits into from
Apr 28, 2024

Conversation

cclauss
Copy link
Contributor

@cclauss cclauss commented Apr 27, 2024

This Action uses minimal steps to run in ~5 seconds to rapidly:

  • use codespell to look for typos in the codebase, and
  • use ruff to lint Python code and provide intuitive GitHub Annotations to contributors.

Tools:

% codespell --write-changes

./docs/index.rst:69: sucessfully ==> successfully
./docs/tips.rst:46: tranferring ==> transferring
./docs/etags.rst:21: presense ==> presence
./cachecontrol/filewrapper.py:41: vaguaries ==> vagaries
./cachecontrol/filewrapper.py:44: thigns ==> things
./tests/test_cache_control.py:178: wih ==> with

% ruff check --target-version=py38

Error: cachecontrol/caches/file_cache.py:9:39: F401 `typing.Union` imported but unused
Error: tests/conftest.py:163:5: E722 Do not use bare `except`
Error: tests/issue_263.py:16:1: E402 Module level import not at top of file
Error: tests/issue_263.py:16:20: F401 `pprint.pprint` imported but unused
Error: tests/test_serialization.py:5:8: F401 `pickle` imported but unused
Error: Process completed with exit code 1.

@woodruffw
Copy link
Member

This repo is already formatted with black, which is declared as a dev-dep and is run via make format. I don't mind changing that to ruff instead, but I would strongly prefer that the switch is done by changing the dev dep + Makefile rather than adding a CI-only dependency that I'm almost certainly going to forget about 🙂

(Same goes for the spellchecker: no objection to having one, but please make it a dev dep.)

.github/workflows/tests.yml Outdated Show resolved Hide resolved
Makefile Outdated
@@ -13,7 +13,9 @@ bootstrap: $(VENV)/bin/pip
$(VENV)/bin/pip install -e .[dev]

format:
$(VENV)/bin/black .
$(VENV)/bin/codespell
$(VENV)/bin/ruff check
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs to be ruff check --fix to perform changes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some folks want ruff to autofix but others do not often because they are used to flake8 which never fixes.

pyproject.toml Outdated Show resolved Hide resolved
@woodruffw
Copy link
Member

Thanks @cclauss, two nitpicks but this LGTM overall 🙂

@woodruffw
Copy link
Member

macOS with Python 3.7 is failing for unrelated reasons: actions/setup-python#856

Co-authored-by: William Woodruff <[email protected]>
Makefile Outdated Show resolved Hide resolved
Comment on lines 21 to 23
exclude:
- python-version: "3.7"
os: "macos-latest"
Copy link
Member

Choose a reason for hiding this comment

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

Let's just leave this out for now -- I'd rather merge this with a broken CI (or wait a little longer for GHA to fix it) than have a matrix exclude rule that I'll likely forget about 🙂

.github/workflows/tests.yml Outdated Show resolved Hide resolved
@woodruffw woodruffw merged commit 6d5049a into psf:master Apr 28, 2024
17 of 18 checks passed
@cclauss cclauss deleted the codespell_and_ruff branch April 28, 2024 14:53
@woodruffw
Copy link
Member

Thanks @cclauss! Please hold off on the CI changes for a bit -- I think what we'll want there is to have a make lint or similar that the CI then runs (using setup-python's ability to cache deps).

@cclauss
Copy link
Contributor Author

cclauss commented Apr 28, 2024

I would highly recommend that you step up to pre-commit instead of make.

@woodruffw
Copy link
Member

I understand that people like pre-commit, but I'm not a huge fan of git hooks-based workflows: IME people frequently fail to configure them, and they require yet another piece of global tooling (pip[x] install pre-commit) that can break between Python versions.

That isn't to say that they're bad or that Makefiles are good (they aren't!), but I find them simpler to reason about. So I'm inclined to keep this repo in a "minimal" state with just a Makefile until Python's developer tooling circularities get resolved.

woodruffw added a commit to woodruffw-forks/cachecontrol that referenced this pull request Jul 9, 2024
* GitHub Action to spellcheck and lint Python code

* Update pyproject.toml

Co-authored-by: William Woodruff <[email protected]>

* $(VENV)/bin/ruff check --fix

* exclude: python-version: "3.7" on os: "macos-latest"

* Update tests.yml

* Allow Python 3.7 on ARM to crash

---------

Co-authored-by: William Woodruff <[email protected]>
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.

2 participants