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.
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.
# Bad: Mutable defaultdefprocess(items: list = []): # Bug!
...
# Good: None defaultdefprocess(items: list | None = None):
items = items or []
...
# Bad: Bare excepttry:
...
except:
pass# Good: Specific exceptiontry:
...
except ValueError as e:
logger.error(e)
# Bad: truthiness guard swallows a legitimate 0 / 0.0 / "" / False
hour = config.get("start_hour") or9# a valid 0 (midnight) silently becomes 9ifself.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 = 9if hour isNoneelse hour
ifself.max_drawdown isnotNone:
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 isnot"": # CPython interning makes it *sometimes* work — never rely on it# Good: value comparisonif 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 effectreturn response.status_code == 403def_is_rate_limited(response): # this is the one that winsreturn response.status_code == 403and"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:
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 losttry:
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:
returnf"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 numberstry:
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"deffetch_all():
items = []
for page in paginate():
try:
items.extend(page)
except RequestError:
break# silent truncationreturn 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 tracefor 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 insorted(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 outsidedefsimulate():
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 rundefsimulate(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
# Iterationfor item in items: # Not: for i in range(len(items))for i, item inenumerate(items): # When index needed# Dictionary access
value = d.get(key, default) # Not: if key in d: value = d[key]# Context managerswithopen(path) as f: # Not: f = open(path); try: finally: f.close()# Comprehensions (simple only)
squares = [x**2for 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
