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.

Sign up for GitHub

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

Jump to bottom

github: switch the python checker CI to use something that is maintained #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

github: switch the python checker CI to use something that is maintained #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c1444c
github: switch the python checker CI to use something that is maintained
eli-schwartz Jan 3, 2024
57d5d2b
github: specify minimum python versions for lint checks
eli-schwartz Jan 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions .github/workflows/python.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,43 @@ name: Python

on: [push, pull_request]

env:
PYTHON_MIN: '3.7'
Copy link
Member

Choose a reason for hiding this comment

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

i added a pyproject.toml file and moved tool versioning info to that, so i think we can drop this now

# Reusable list of files to check -- parsed by bash, so does not support
# whitespace.
CHECK_FILES: lddtree.py pylint

jobs:
python:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
# NB: v1.4.0 covers Python 3.8.
- uses: ricardochaves/python-lint@v1.4.0
- uses: actions/setup-python@v5
with:
python-version: '3.x'
Copy link
Member

Choose a reason for hiding this comment

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

if we have control over versions, then we'd want to test a range of versions, starting with 3.6 if possible

Copy link
Member Author

Choose a reason for hiding this comment

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

Lint tools are usually python version agnostic: they'll provide the same style violation reports regardless of which python version the linter tool is installed for.

The linter tools will, in some cases, implement different checks depending on version that you want to be compatible with. For example:

mypy --python-version=3.11 lddtree.py

will tell mypy to type check against the 3.11 version of the standard library type checking stubs.

pylint --py-version=3.5 lddtree.py

will tell pylint to not e.g. suggest using an f-string.

Copy link
Member

Choose a reason for hiding this comment

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

if that's reliable, that's fine, but it's also something we need in here

Copy link
Member Author

Choose a reason for hiding this comment

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

So to be clear, do you want:

  • linters that support checking against multiple python versions, to do so?
    • lower and upper version bounds only? e.g. 3.6 and 3.12?
  • linters that support checking against multiple versions, to check against the lowest supported version to avoid suggesting issues that cannot be fixed until bumping the minimum version?

Copy link
Member

Choose a reason for hiding this comment

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

for black/isort, we just need to tell them the min version to use as the syntax, so setting it to py3.6 alone is OK

for pylint/mypy, yes, we'd want to check multiple versions. ideally it'd be 3.6 as the min, but if that's onerous, i'd be OK raising this to py3.8. but not beyond that atm.

i guess i'd be OK with a min/max version, but seems like if we could do multiple versions, then it'd be easy to list them all inclusive. i.e. use github actions matrix settings.

Copy link
Member

Choose a reason for hiding this comment

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

i updated the requirement to python 3.8 now

i also added requirements.txt files to test here

- uses: actions/cache@v3
with:
python-root-list: lddtree.py pylint
eli-schwartz marked this conversation as resolved.
Show resolved Hide resolved
use-pylint: true
use-pycodestyle: false
use-flake8: false
use-black: true
use-mypy: true
use-isort: true
extra-pylint-options: ""
extra-pycodestyle-options: ""
extra-flake8-options: ""
extra-black-options: ""
extra-mypy-options: ""
extra-isort-options: ""
path: ~/.cache/pip
key: lint-pip-${{ github.run_number }}
Copy link
Member

Choose a reason for hiding this comment

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

doesn't tying this to the run_number defeat the cache ?

wouldn't we want to cache based on the python version and hash of the relevant inputs ? in this case, requirements*.txt ?

restore-keys: lint-pip-
- name: install dependencies
run: |
pkgs=(
pylint mypy black 'isort[colors]'
# mypy dependencies for following imported types.
argcomplete
)
python -m pip install "${pkgs[@]}"
- run: pylint --py-version=$PYTHON_MIN --output-format colorized $CHECK_FILES
- run: |
mypy $CHECK_FILES
# Also check against the oldest supported python
Copy link
Member

Choose a reason for hiding this comment

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

nit: period at the end

mypy --python-version=$PYTHON_MIN $CHECK_FILES
if: success() || failure()
env:
TERM: xterm-color
MYPY_FORCE_COLOR: 1
- run: black --check --diff --color $CHECK_FILES
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 add --verbose so the log contains config info

if: success() || failure()
- run: isort --check --diff --color $CHECK_FILES
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 add --verbose so the log contains config info

if: success() || failure()
2 changes: 1 addition & 1 deletion lddtree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
# Disable import errors for all 3rd party modules.
# pylint: disable=import-error
try:
import argcomplete # type: ignore
import argcomplete
except ImportError:
argcomplete = cast(Any, None)

Expand Down