improving-python-code-quality

name	improving-python-code-quality
description	Improves Python library code quality through ruff linting, mypy type checking, Pythonic idioms, and refactoring. Use when reviewing code for quality issues, adding type hints, configuring static analysis tools, or refactoring Python library code.

Python Code Quality

Quick Reference

Tool	Purpose	Command
ruff	Lint + format	ruff check src && ruff format src
mypy	Type check	mypy src

Ruff Configuration

Minimal config in pyproject.toml:

[tool.ruff]
line-length = 88
target-version = "py310"

[tool.ruff.lint]
select = ["E", "W", "F", "I", "B", "C4", "UP"]

For full configuration options, see RUFF_CONFIG.md.

MyPy Configuration

[tool.mypy]
python_version = "3.10"
disallow_untyped_defs = true
warn_return_any = true

For strict settings and overrides, see MYPY_CONFIG.md.

Type Hints Patterns

# Basic
def process(items: list[str]) -> dict[str, int]: ...

# Optional
def fetch(url: str, timeout: int | None = None) -> bytes: ...

# Callable
def apply(func: Callable[[int], str], value: int) -> str: ...

# Generic
T = TypeVar("T")
def first(items: Sequence[T]) -> T | None: ...

For protocols and advanced patterns, see TYPE_PATTERNS.md.

Common Anti-Patterns

# Bad: Mutable default
def process(items: list = []):  # Bug!
    ...

# Good: None default
def process(items: list | None = None):
    items = items or []
    ...

# Bad: Bare except
try:
    ...
except:
    pass

# Good: Specific exception
try:
    ...
except ValueError as e:
    logger.error(e)

# Bad: truthiness guard swallows a legitimate 0 / 0.0 / "" / False
hour = config.get("start_hour") or 9   # a valid 0 (midnight) silently becomes 9
if self.max_drawdown:                  # max_drawdown=0 silently disables the limit
    enforce(self.max_drawdown)

# Good: guard on None, not on truthiness
hour = config.get("start_hour")
hour = 9 if hour is None else hour
if self.max_drawdown is not None:
    enforce(self.max_drawdown)

x = x or default is fine only when the single falsy value you mean to replace is an empty container (e.g. items = items or []). For numeric, boolean, or string fields where 0, False, or "" are meaningful inputs, it is a bug — use x if x is not None else default.

# Bad: identity comparison against a literal (ruff flags this as F632)
if name is not "":   # CPython interning makes it *sometimes* work — never rely on it

# Good: value comparison
if name != "":

A Rule You Enabled Isn't a Rule That Fires

Linters ship exemptions, and the costly ones are invisible: the rule is in select, the job is green, and the class of mistake you believed was policed walks straight through. Before relying on a check to protect something, write the violation on purpose once and confirm it gets reported.

The Ruff default that bites hardest is dummy-variable-rgx. It exists so _-prefixed throwaways don't trip unused-variable checks, but its default pattern matches any leading-underscore name — so F811 (redefinition of an unused name) ignores every private helper in the codebase.

# module.py — two module-level definitions. The FIRST one is dead code.
def _is_rate_limited(response):          # never called; editing it has no effect
    return response.status_code == 403

def _is_rate_limited(response):          # this is the one that wins
    return response.status_code == 403 and "rate limit" in response.text.lower()

ruff check --select F811 passes clean on that file. Rename both to is_rate_limited and it fires immediately (verified on ruff 0.16). The gate is working — the name opted out of it.

This shape shows up most often after two branches independently add the same module-level helper and a conflict resolution keeps both sides. Nothing goes red; the bodies usually agree at first; then someone patches the dead copy and cannot work out why the behaviour didn't change.

Prefer fixing it in config, so the check actually covers private names:

[tool.ruff.lint]
dummy-variable-rgx = "^_$"   # only a bare `_` is a throwaway

State the tradeoff honestly before adopting it: _, keep = pair() stays silent, but _unused = compute() now trips F841. In a codebase that leans on _name throwaways that is real noise — delete the assignment or use a bare _ rather than reverting the regex.

Independently, after resolving a conflict in a module both branches edited, look for duplicated definitions directly — this catches shadowing the linter's config can't:

grep -oE '^(def|class) [A-Za-z_][A-Za-z0-9_]*' module.py | sort | uniq -d

The habit generalizes past Ruff: when a check is load-bearing, introduce the mistake once and watch it get caught, the same way a regression test is only trustworthy after you've seen it go red.

Fail Loud: Don't Degrade Silently

The costliest bugs aren't crashes — they're failures that look like success. Code that swallows an error and returns something plausible corrupts data downstream with no signal that anything went wrong. Prefer raising, or at minimum surfacing an explicit error, over a quiet fallback.

Don't collapse exceptions into a generic string — and don't discard output.

# Bad: real parse error becomes a one-line string; traceback and context lost
try:
    return parse(path)
except Exception as e:
    return {"error": str(e)}     # every distinct failure looks the same

# Bad: a nonzero exit discards stdout — where many CLIs write their real summary
result = subprocess.run(cmd, capture_output=True, text=True)
if result.returncode != 0:
    return f"Error: {result.stderr}"   # tools that exit nonzero *by design*
                                       # (e.g. "N findings") report empty here

# Good: let it raise (or return stdout+stderr+code so the caller can decide)
return {"stdout": result.stdout, "stderr": result.stderr, "code": result.returncode}

Give success and failure distinct sentinels. If the error path writes the same status value the success path uses, failed runs read as complete.

# Bad: on failure, status is set to the SAME value that means "done"
try:
    run_job()
    status = "READY"
except Exception:
    status = "READY"     # failures are now indistinguishable from success

# Good: a terminal error state the UI/caller can branch on
    status = "ERROR"

Never substitute fabricated data on failure. Returning randomized or sample data when a fetch fails makes the UI show plausible-but-invented numbers.

# Bad: a network hiccup silently becomes made-up numbers
try:
    return fetch_metrics()
except RequestError:
    return generate_sample_data()   # user can't tell real from fake

# Good: propagate the failure (or return an explicit sentinel the UI renders as an error)

Signal partial results — don't return them as complete. A paginated fetch that aborts mid-stream and returns what it has looks identical to a full result.

# Bad: caller can't distinguish "12 items" from "12 of 900 before the API died"
def fetch_all():
    items = []
    for page in paginate():
        try:
            items.extend(page)
        except RequestError:
            break            # silent truncation
    return items

# Good: return completeness alongside the data (or raise)
    return items, complete   # caller warns loudly when complete is False

Batch loops: collect per-item errors, don't just continue. Skipping bad inputs silently gives no way to know coverage was incomplete — and makes sibling operations inconsistent when some report errors and others don't.

# Bad: files that fail to load vanish with no trace
for f in files:
    try:
        process(load(f))
    except Exception:
        continue                     # how many were skipped? which ones?

# Good: accumulate skips and return them so callers (and automation) can see them
errors = []
for f in files:
    try:
        process(load(f))
    except Exception as e:
        errors.append({"file": f, "error": str(e)})
return {"processed": ..., "errors": errors}

The unifying rule: when you catch an error, either recover meaningfully or make the failure visible (raise, log at error level, or return a distinguishable sentinel). A return/continue/fallback inside except that produces normal-looking output is where silent corruption lives.

Determinism & Reproducibility

Non-deterministic output is a quiet tax: churny, unreviewable diffs; flaky tests; and simulations no one can reproduce from a seed. Three sources recur.

Serializing from an unordered container. Building a list or JSON payload by iterating a set (or merging into a dict and ranging it) emits results in an order that varies run to run — str hashing is randomized per process, so a regenerated file is the same data reshuffled, and the real change drowns in noise in a repo whose whole point may be a clean diff. Impose a total order before you serialize.

# Bad: set iteration order isn't stable across runs
tags = [render(t) for t in tag_set]
json.dump(record, f)                     # nested sets/merges churn the output

# Good: sort before writing
tags = [render(t) for t in sorted(tag_set)]
json.dump(record, f, sort_keys=True)

Multiple RNGs, none injectable. A library that draws from both random and numpy.random needs both seeded to be reproducible — seeding one leaves the other free-running, so the run is only half-deterministic. And seeding the global RNGs (random.seed, np.random.seed) clobbers the caller's global state. Accept a seed (or an RNG instance) and thread local generators through, so callers get reproducibility without you reaching into their globals.

# Bad: two independent global RNGs, no way to seed from the outside
def simulate():
    x = random.random()              # stdlib global stream
    y = np.random.normal()           # numpy global — a *separate* stream

# Good: caller-supplied, local generators; one seed reproduces the whole run
def simulate(seed: int | None = None):
    rng = random.Random(seed)
    nprng = np.random.default_rng(seed)
    x = rng.random()
    y = nprng.normal()

Unpinned parsing context. Locale-dependent parsing — datetime.strptime with %b/%a (month/day names) — silently changes behavior across machines. Pin the format and locale so a test that passes on your box passes in CI too.

Non-determinism also weakens tests: a function that shuffles its output forces assertions so loose ("contains any of these words") that they stop catching regressions. Make the seam deterministic — inject the RNG — and assert exact output.

Pythonic Idioms

# Iteration
for item in items:           # Not: for i in range(len(items))
for i, item in enumerate(items):  # When index needed

# Dictionary access
value = d.get(key, default)  # Not: if key in d: value = d[key]

# Context managers
with open(path) as f:        # Not: f = open(path); try: finally: f.close()

# Comprehensions (simple only)
squares = [x**2 for x in numbers]

Module Organization

src/my_library/
├── __init__.py      # Public API exports
├── _internal.py     # Private (underscore prefix)
├── exceptions.py    # Custom exceptions
├── types.py         # Type definitions
└── py.typed         # Type hint marker

Checklist

Code Quality:
- [ ] ruff check passes
- [ ] mypy passes (strict mode)
- [ ] Public API has type hints
- [ ] Public API has docstrings
- [ ] No mutable default arguments
- [ ] Specific exception handling
- [ ] Failures fail loud — no fabricated fallbacks, colliding success/error sentinels, or silently-truncated results
- [ ] Batch loops collect per-item errors instead of a bare `continue`
- [ ] Truthiness guards don't swallow valid 0/False/"" (guard on `is None`)
- [ ] No `is`/`is not` against literals (use ==/!=)
- [ ] Load-bearing lint rules verified to fire (F811 skips `_`-prefixed names under the default `dummy-variable-rgx`)
- [ ] No duplicate module-level `def`/`class` names after a conflict resolution
- [ ] Deterministic output: sort before serializing; seed/inject all RNGs (both `random` and `numpy`)
- [ ] py.typed marker present

Learn More

This skill is based on the Code Quality section of the Guide to Developing High-Quality Python Libraries by Will McGinnis. See these posts for deeper coverage:

• Linting & Formatting with Ruff
• Understanding McCabe Complexity
• Adding Type Hints