Skip to content

Run flake8 as a module of the running interpreter - #10

Merged
nelson2005 merged 1 commit into
mainfrom
fix/codeblock-flake8-module
Jul 31, 2026
Merged

Run flake8 as a module of the running interpreter#10
nelson2005 merged 1 commit into
mainfrom
fix/codeblock-flake8-module

Conversation

@nelson2005

Copy link
Copy Markdown
Owner

extract_codeblocks.py invoked a bare flake8, so the doc-codeblock check only ran where flake8 happened to be on PATH. Running it from a virtualenv python — the normal local case — died with FileNotFoundError: [Errno 2] No such file or directory: 'flake8' before linting a single block.

It now runs flake8 as a module of whichever interpreter executes the script ([sys.executable, '-m', 'flake8']), so a venv python finds its own flake8 with no PATH setup. --flake8 EXE still overrides for an explicitly chosen binary.

CI behaviour is unchanged: doc-codeblock-flake8.yml pip-installs flake8 into the same interpreter it then runs the script with, so both the old and new invocation resolve there.

This mirrors the canonical template in nelson2005/config#63; this repo's SKIP_DIRS customization is deliberate and untouched.

Verified against a seeded fixture pair with flake8 removed from PATH:

case result
clean block exit 0 (previously FileNotFoundError)
block with an unused import exit 1, docs/dirty.rst:6:1:1: F401
explicit --flake8 EXE exit 1 — override still honoured
interpreter without flake8 exit 1, No module named flake8

This repo's own docs pass unchanged.

extract_codeblocks.py shelled out to a bare 'flake8', so it only worked when
flake8 happened to be on PATH. Run it as '<sys.executable> -m flake8' instead,
which resolves flake8 from whichever interpreter runs the script -- a virtualenv
python now works with no PATH setup.

--flake8 EXE still overrides, so an explicitly chosen binary is unaffected. CI is
unaffected either way: it pip-installs flake8 into the same interpreter it then
runs the script with.
Copilot AI review requested due to automatic review settings July 31, 2026 03:36
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@github-actions

Copy link
Copy Markdown

MiniMax Code Review

The user wants me to review a pull request that modifies the file `.github/scripts/extract_codeblocks.py`. Let me analyze the changes:
  1. The function lint_block signature changed from flake8_exe to flake8_cmd - this suggests it's now expecting a list instead of a string.

  2. In lint_block:

    • Changed from [flake8_exe, f'--max-line-length={max_line_length}', ...] to [*flake8_cmd, f'--max-line-length={max_line_length}', ...]
    • This means flake8_cmd is now expected to be a list (due to the unpacking operator *)
  3. In main:

    • The --flake8 argument now defaults to None instead of 'flake8'
    • Added a new help text explaining the default behavior
    • Added a new line: flake8_cmd = [args.flake8] if args.flake8 else [sys.executable, '-m', 'flake8']
    • Changed the call to lint_block from passing args.flake8 to passing flake8_cmd

Let me review this for potential issues:

Positive aspects:

  1. The change allows the script to use the Python interpreter that's running the script to invoke flake8 as a module (python -m flake8). This is useful in virtual environments where flake8 might not be on PATH but is installed in the virtualenv.
  2. The explicit --flake8 argument allows users to override this behavior if needed.

Potential issues:

  1. Bug in list construction: When args.flake8 is provided, it's treated as a single string but wrapped in a list: [args.flake8]. This works fine - it creates a list with one element.

  2. Edge case with empty string: If someone passes --flake8 '' (empty string), the condition if args.flake8 would be falsy (empty string is falsy), so it would fall back to [sys.executable, '-m', 'flake8']. This is probably fine, but could be confusing. However, since the argument parser doesn't specify allow_empty=True or similar, this might not be a real concern.

  3. Security: This change doesn't introduce any security issues - it's just constructing command-line arguments for subprocess.

  4. The variable name change: flake8_exe was renamed to flake8_cmd which is more accurate since it's now a list (a command) rather than just an executable path.

  5. Testability: The change actually improves testability - you can now pass a list like ['echo', 'mock'] for testing purposes, though that's not what the argument parser does.

One thing I notice is that this is a relatively minor change that makes the script work better in virtualenv environments. The code looks correct to me. Let me double-check the logic:

flake8_cmd = [args.flake8] if args.flake8 else [sys.executable, '-m', 'flake8']

This means:

  • If args.flake8 is provided and is truthy (non-empty string), use [args.flake8]
  • Otherwise, use [sys.executable, '-m', 'flake8']

This seems correct. The original code would have just used flake8 as the executable name, which would only work if flake8 was in PATH. The new code uses the Python interpreter that's running the script to invoke flake8 as a module, which should work in most virtualenv setups.

Code Review Summary

This change improves the script to work better in virtual environments by invoking flake8 as a Python module rather than relying on it being in PATH.

Changes Made

  1. Default behavior change: Instead of defaulting to 'flake8' (requiring it to be on PATH), the script now defaults to python -m flake8 using the same interpreter running the script
  2. Function signature update: flake8_exe parameter renamed to flake8_cmd and now accepts a list
  3. Argument parsing: Added logic to construct the command list based on whether --flake8 is provided

Potential Issues

Minor: Empty string handling

If a user explicitly passes --flake8 '' (empty string), the condition if args.flake8 evaluates to False (empty string is falsy), so it falls back to the module-based invocation. This might be unexpected:

flake8_cmd = [args.flake8] if args.flake8 else [sys.executable, '-m', 'flake8']

Consider using if args.flake8 is not None: if empty string should be treated as an explicit executable name (though this is an edge case).

Verdict

Approved - The change is clean, well-documented, and solves the intended problem (running flake8 in virtualenvs without PATH setup). The only suggestion would be to clarify whether empty string for --flake8 should be treated as an explicit value or fall back to default, but this is a very minor edge case.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Ready to approve

The change is small, localized, and correct, and it removes a concrete failure mode in virtualenv usage without altering the script’s core behavior.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR hardens the doc code-block linting script by invoking flake8 via the same Python interpreter that is running extract_codeblocks.py (i.e., sys.executable -m flake8) instead of relying on a flake8 binary being present on PATH. This aligns the script with typical virtualenv usage while preserving an explicit override via --flake8.

Changes:

  • Switch flake8 invocation from a bare executable name to a command list that defaults to [sys.executable, '-m', 'flake8'].
  • Preserve the ability to override flake8 via --flake8, now treated as an optional explicit executable.
File summaries
File Description
.github/scripts/extract_codeblocks.py Builds flake8_cmd and uses it for subprocess.run, avoiding failures when flake8 isn’t on PATH but is installed in the running interpreter’s environment.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@nelson2005
nelson2005 merged commit de88ca5 into main Jul 31, 2026
22 checks passed
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