Python Code Review & Refactor

The reasoning

A type hint is a contract, and this one is unsatisfiable. Anything genuinely `Iterable` — a generator, a file handle, a `zip`, a database cursor — is drained by the first `sum` and yields nothing to the second pass. The result would be a count of 0 alongside a correct total: no error, just a wrong number.

The reason it works today is that every caller happens to pass a list, which is re-iterable. That is the definition of a latent defect: correct by coincidence, and the coincidence is not written down anywhere. A type checker will not flag it, because passing a list where an `Iterable` is expected is perfectly valid — the mismatch is between the annotation and the body, which no tool checks.

Narrowing to `Sequence` is the smaller change and makes the requirement explicit: callers must pass something with a length and stable ordering, and `len(rows)` becomes honest rather than a hidden second traversal. A generator caller now fails at the type checker instead of at run time with a wrong answer.

The alternative — one pass accumulating both values — keeps the wider `Iterable` contract and works on a stream of any size. Which to choose depends on whether streaming matters here. The reviewable point is that the current code chose neither, and picked an annotation that matches the callers rather than the implementation.

The reasoning

`dict[str, Any]` is the type-system equivalent of no annotation. Neither a reader nor a checker can tell whether the result has `region` or `regions`, whether `amount` is an `int` or a `Decimal`, or whether `id` is guaranteed present. Every consumer five stages downstream has to read this function to find out, and a renamed key is discovered at run time.

A dataclass moves all of that into one declaration. `row.region` is checked by mypy and autocompleted by the editor; a typo is an error before the code runs rather than a `KeyError` in production. The definition itself becomes the documentation, which is worth more than a docstring because it cannot drift.

Making it `frozen=True` adds two things worth having: the record cannot be mutated halfway down the pipeline by a stage that "just adds a field", and it becomes hashable, so it can key a dict or join a set for deduplication. Both are common needs and both are unavailable on a plain dict.

The honest counter-argument is the boundary. Data arriving from JSON or a CSV genuinely is an untyped dict, and forcing it into a typed record before validating just moves the failure. The pattern that works is a narrow untyped edge — parse to a dict, validate, construct the dataclass — and a typed interior. `asdict()` converts back when something downstream needs a plain dict for serialisation, which is why the output here is unchanged.

The reasoning

The snippet shows the failure directly: `batch_sze=500` and `nonsense=True` are both accepted without complaint, and both produce a run using the defaults. Nothing raises, nothing logs, and the difference between "I configured a batch of 500" and "I configured nothing" is invisible in the output.

An explicit signature makes the same call a `TypeError` at the call site, before any work happens. It also gives you the three things reviewers rely on: the parameter list is the documentation, defaults are visible in one place, and a type checker can verify that `batch_size` is an `int` rather than the string that arrived from an environment variable.

`**kwargs` earns its place in exactly two situations — a decorator or wrapper forwarding an arbitrary signature it does not own, and an API that genuinely accepts open-ended options destined for something else, such as driver-specific connection parameters. Both are pass-through cases. Using it to avoid writing out four parameters is not one of them.

Where it is genuinely needed, narrow the damage: validate the keys against a known set and raise on anything unrecognised, or use a `TypedDict` with `total=False` so a checker can still see the option names. Silently ignoring an unknown key is a choice, and it is almost never the right one for configuration.

The reasoning

The current code cannot distinguish "this status is not final" from "this is not a status". Both return False, and the second is a data-quality problem that will keep returning False for every row from that source. A dashboard reports zero cancellations and nothing anywhere reports an error.

A `StrEnum` fixes it at the boundary: constructing `Status("canceled")` raises `ValueError`, so the unknown value is caught at the point it enters the system, with the offending string in the message. Because `StrEnum` members are also `str`, existing comparisons, JSON serialisation and database writes keep working — which is why the printed output here is unchanged.

The static half matters as much. With an enum or `Literal["paid", "pending", ...]` in the signature, a type checker rejects a misspelled literal in the source before anything runs, and an editor autocompletes the valid values. The set of legal statuses becomes something you can find by going to a definition rather than by grepping for string literals.

The reviewable judgement is how to handle the unknown value, and it is worth being explicit that the rewrite here catches it and returns False — preserving behaviour, as a refactor must. In the real change you would decide deliberately: raise and fail the row, or map to an `UNKNOWN` member and count it. Both are defensible; silently returning False for a value nobody recognises is not.

The reasoning

The five things `print` cannot do, in the order they hurt. Levels: there is no way to emit diagnostics that are off by default and switchable on during an incident, so debug output either ships permanently or does not exist. Configuration: the destination is baked in at the call site rather than chosen by whoever runs the code. Context: no timestamp, no module, no line, so a line in a log file cannot be traced back. Exceptions: no `exc_info`, so a traceback must be assembled by hand. Structure: no way to attach fields that a log aggregator can index and filter.

The last one is what matters at scale. `logger.info("loaded %s rows", n, extra={"dataset": ds, "batch": batch_id})` produces a record that a JSON formatter turns into queryable fields, so "show me every failed batch for this dataset today" is a search rather than a grep. `print` produces a line of text that a human has to read.

The setup discipline is worth stating too, because it is where teams get it wrong: libraries and modules call `logging.getLogger(__name__)` and configure nothing, while the *application entry point* configures handlers, levels and formatters exactly once. A module that calls `basicConfig` at import time hijacks logging for everyone who imports it.

The counter-case is real and small: a one-off script, or a CLI whose output *is* the product — a report, a JSON document meant to be piped. There, `print` to stdout is correct, and diagnostics should still go to a logger on stderr so the two streams stay separable.

The reasoning

The snippet shows it directly: the first captured record contains no traceback, the second does, and the second names the call that raised. That difference is the whole distance between "the load failed" and "the load failed at line 34, parsing a value of 'twenty'".

`logger.exception(msg)` is shorthand for `logger.error(msg, exc_info=True)` and is only valid inside an exception handler, where it picks up the current exception implicitly. Note that it logs at ERROR level — if you want a traceback at WARNING or CRITICAL you pass `exc_info=True` explicitly, which is the more general form.

Because the traceback is attached to the record rather than interpolated into the message, a JSON formatter can put it in its own field. That keeps the message stable and greppable — every occurrence reads `load failed` — while the varying detail stays queryable. Interpolating `str(error)` into the message does the opposite: every occurrence is a slightly different string, so counting them is hard.

The related habit is to log the exception exactly once, at the level that can do something about it. Logging and re-raising at every layer produces the same failure five times with five tracebacks, which makes an incident log harder to read rather than easier. Catch, log with `exception`, and either handle it or let it propagate — not both.

The reasoning

The mechanics are ordinary Python: arguments are evaluated before the call. `f"payload: {payload}"` builds the string, calls `__str__` on everything interpolated, and hands the finished text to `debug()`, which checks the level and discards it. The work is already done.

The `%s` form passes the object itself and lets the logging module interpolate only if a handler will emit the record. The snippet counts the renders to prove it: the f-string call renders once with DEBUG disabled, the `%s` debug call renders zero times, and the `%s` info call renders because INFO is enabled and the record is actually written.

Whether that matters depends entirely on what is being formatted. For a small int it is noise. For a row, a payload, a dataframe summary, or anything whose `__repr__` walks a structure, it is real work per call — and debug logging tends to sit in the hottest loops precisely because that is where you want detail. A disabled debug line in a per-row loop can cost more than the row processing.

Two follow-ons worth knowing. `logger.isEnabledFor(logging.DEBUG)` guards genuinely expensive preparation that the `%s` form cannot defer, such as building a summary object. And linters check this — `ruff` flags f-strings in logging calls as `G004` — so the rule can be enforced rather than remembered.

The reasoning

The test for a log line is whether it answers the question you will have at 3am. "Failed to parse row" answers "did something fail?" and nothing else. You cannot find the file, cannot reproduce it, cannot tell whether the 4,000 failures are one bad file or sixty slightly bad ones — which is the first thing you would want to know, because those have completely different causes.

The fields to attach are the ones you would use to reproduce it: the dataset, the source path, the row or byte offset, the run or batch id, and the reason. Put them in `extra=` rather than in the message so a JSON formatter emits them as indexed fields; then "group by source_path" is a query, and the one bad file falls out immediately.

Keeping the message constant is the other half. `logger.warning("row rejected", extra={...})` produces 4,000 records with one message and varying fields, which aggregates cleanly. Interpolating the detail into the message gives 4,000 distinct strings, and every dashboard that counts by message sees 4,000 unique events.

Two practical additions. Volume: 4,000 identical warnings is its own problem — log the first few per file and a summary count at the end, rather than one line per row. And redaction: the row is the obvious thing to attach and often the one thing you must not, since it may carry personal data. Log the identifiers and the reason; log the payload only behind a debug flag, and never by default.

The reasoning

Constructing the dependency inside the function makes it invisible to the caller and unavoidable in a test. The only way in is `unittest.mock.patch("module.Client")`, which is brittle in a specific way: it depends on where the name is imported, so moving the import or renaming the module breaks tests that have nothing to do with the change.

Passing the client in makes the dependency part of the contract. The test constructs a two-line fake and calls the function directly — no patching, no import-path knowledge, and the test now exercises the real code path rather than a mocked one. It also documents what the function needs, which the original signature actively hid.

Typing the parameter as a `Protocol` rather than as the concrete class keeps the dependency pointing the right way: the function declares what it needs (`something with .get(path)`), and neither the real client nor the fake has to import anything from it. That is the structural-typing argument from the Conceptual bank, in the situation where it pays off.

The counter-argument to pre-empt: injection can be taken too far, and threading a dependency through six layers is worse than constructing it once. The rule that works is to inject at the boundary — anything that does I/O, reads the clock, or generates randomness — and construct everything else where it is used. Those three are exactly the things that make tests slow, flaky or impossible.

The reasoning

The test problem is the visible one. An assertion about the output depends on when it runs, so you either freeze the clock with `freezegun` or `unittest.mock.patch`, or you write a test that computes the expected value the same way the code does — which asserts nothing at all.

The backfill problem is the serious one, and it is about correctness rather than convenience. Every orchestrator distinguishes the *logical* date a run represents from the wall-clock time it executes; that is what `{{ ds }}` in Airflow, `dbt`'s `run_started_at`, and the equivalent elsewhere are for. Code that calls `now()` internally ignores the distinction, so re-running a failed run from three days ago writes into today's partition and leaves the original gap unfilled.

Passing the date in makes the function a pure function of its arguments: same input, same output, forever. That is what makes it testable, and it is the same property that makes the job idempotent and re-runnable — the two are not separate benefits, they are the same benefit seen from two angles.

The same argument covers the other sources of ambient state: random values, UUIDs, the hostname, the current working directory. Inject them at the boundary, or generate them once at the entry point and thread the value down. A function that reaches for any of them is a function whose output you cannot predict from its input.

The reasoning

The mixed version can only be tested end to end: build a file or a `StringIO`, run the whole thing, capture stdout, and assert on text. That test is slow to write, fragile against any change to the printed format, and tells you nothing about *which* stage was wrong when it fails.

Once the transformation is a function over an iterable, testing it is a one-liner — `assert eu_total([("eu", 10), ("us", 20)]) == (1, 10)`. There is no setup, it runs in microseconds, and a failure points at exactly one stage. The parsing gets its own tests for the cases that actually break in production: a ragged line, a missing field, an unparseable number.

The generator shape is what makes the split free rather than costly. Each stage yields rather than accumulates, so the composed pipeline still processes one row at a time and peak memory is unchanged — you have not traded efficiency for testability, which is the objection people usually raise.

The remaining `load` is worth looking at: it is three lines of wiring with no branching and no arithmetic. That is the goal — push everything that can be wrong into functions that can be tested cheaply, and leave an I/O shell so thin that an end-to-end test of it is a formality rather than the only line of defence.

The reasoning

A test that patches `_parse_row` and `_build_key` asserts that those functions are called with particular arguments. That is a statement about *how* the code is structured, not about what it produces, so renaming or inlining a helper breaks the test while the behaviour is identical. The suite has become a change-detector rather than a safety net.

The inverse failure is worse and less obvious: because the helpers are mocked, their real behaviour is never exercised. A bug inside `_parse_row` cannot fail this test. The suite is simultaneously fragile and permissive, which is the worst combination — high maintenance cost, low confidence.

The boundary is the line where you stop owning the code: the network, the filesystem, the database, the clock, the random number generator, third-party services. Substitute those, because they are slow, flaky or impossible in CI. Everything on your side of the line should run for real, so the test covers the code you actually wrote.

Two techniques make it easy. Inject the boundary rather than patching it, as in the client question, so the test passes a fake through the front door. And prefer a small hand-written fake to a `MagicMock`: a fake with real behaviour catches misuse, whereas a `MagicMock` returns a new mock for any attribute you touch and will happily accept a call that would fail against the real thing.

The reasoning

The snippet shows both halves: after changing `BATCH_SIZE` to 500, the early-bound version still returns 100 and the late-bound one returns 500. This is the same rule as the mutable-default trap — `def` is executable code and its default expressions run exactly once — applied to something that looks like configuration rather than like state.

The consequences go beyond the surprise. A test that sets an environment variable in a fixture has no effect, because the module was imported before the fixture ran. Import order becomes significant, which is a horrible thing to depend on. And `os.environ["BATCH_SIZE"]` at import time raises `KeyError` during the import itself if the variable is unset, producing a stack trace that points at an import statement rather than at a missing configuration value.

Reading inside the function fixes the timing but scatters `os.environ` calls through the codebase, so there is no single place to see what the job is configured with and no single place to validate it. The better shape is to load configuration once at the entry point into a frozen settings object — hand-rolled with a `classmethod` `from_env`, or with `pydantic-settings` — and pass it down.

That gives you the three properties you actually want: one place that knows which variables exist, validation and type conversion in one step with an error message naming the variable, and a plain object that a test can construct directly without touching the environment at all.

The reasoning

The immediate cost is that the code can only ever do one thing. Running against a staging bucket, a developer sandbox, or a test fixture requires editing the source — so nobody does, and the pipeline is only ever exercised against production. That is also why a mistake here is expensive: a test run that accidentally writes to `prod-warehouse` is a data incident.

A frozen settings dataclass puts every environment-dependent value in one place with a declared type and a default. That single definition answers "what does this job need to run?" — a question that otherwise requires reading every module — and it becomes the natural place to add validation, so a malformed bucket name fails at startup rather than at the first write.

Passing settings as a parameter rather than reading a module-level global keeps the code testable: a test constructs `Settings(bucket="test-bucket")` and needs no environment, no patching and no fixtures. It also makes the dependency visible in the signature, which is the same argument as injecting a client.

The layering that works in practice: defaults in the dataclass, overridden by a config file, overridden by environment variables, overridden by command-line flags — resolved once at the entry point. `pydantic-settings` implements exactly that and gives you type conversion and error messages naming the offending field. Note that secrets are the one thing that should never have a default: a missing credential must fail loudly, not fall back to something.

The reasoning

The failure mode is specific and nasty. Nothing in your repository changed; a dependency published a new version; the image is rebuilt for an unrelated deploy; the job now behaves differently. The change is real and invisible in your git history, which makes it one of the harder incidents to diagnose — you look for a commit and there is not one.

The distinction that matters is between *declaring* and *locking*. `pyproject.toml` declares what you are compatible with — `pandas>=2.1,<3` says what the code expects and leaves room for patches. The lockfile records what was actually resolved, including every transitive dependency and its hash, so an install reproduces a known-good set exactly. You need both: only ranges and you are not reproducible, only pins and you cannot upgrade anything without editing them by hand.

Pinning direct dependencies with `==` and no lockfile is the common half-measure and it does not work, because your transitive tree is still floating. A pinned `pandas==2.2.1` will happily pull whatever `numpy` it resolves today, and that is enough to change behaviour.

`uv` is the current standard for this and produces `uv.lock` with hashes; `poetry` and `pip-tools` do the equivalent. Commit the lock, install with `--frozen` in CI and in the image build so a drifted lock fails rather than silently re-resolving, and upgrade deliberately with a command that updates the lock and runs the tests. The point is that upgrades become a reviewable commit rather than a side effect of the clock.

The reasoning

An import should define things, not do things. Once it connects, reads a file or calls an API, every consumer inherits that cost and that requirement: the test suite needs a live database to collect tests, a `--help` invocation pays a connection round trip, and a documentation generator that imports your modules fails outright.

The error reporting is bad in a specific way. A `FileNotFoundError` raised during import produces a traceback whose top frame is an `import` statement, often several layers away in something that imported you transitively. The actual problem — a missing config path — is buried, and the module that caused it may not be obvious from the stack at all.

Multiprocessing makes it concrete. Under the spawn start method each worker re-imports your module, so a module-level connection becomes N connections, and a module-level `Pool()` outside an `if __name__ == "__main__":` guard recursively spawns processes. That guard exists precisely because import-time side effects are dangerous.

The fix is to make the work explicit and lazy: a `get_connection()` that creates on first call and caches, or a `main()` that builds everything and passes it down. `functools.lru_cache` on a zero-argument factory is a neat way to get a lazily-created singleton. The test for whether you have got it right is simple — importing the module should be safe with no database, no network and no config file present.

The reasoning

The split is not by line count, it is by responsibility, and the give-away is the shape of the original: three loops, each of which builds a complete intermediate value before the next begins. Those are the seams, and the names — `parse`, `totals_by_region`, `render` — were already implicit in the comments you would otherwise have to write.

What you gain first is testability, for the same reason as the earlier extraction question: `totals_by_region` can be driven with a literal list and asserted in one line, where `run` needs a handle and stdout capture. What you gain second is that each function can fail for one reason, so a traceback names the stage.

Reuse is the third gain and the one people over-claim, so it is worth being precise: `render` and `totals_by_region` are genuinely useful elsewhere, while `parse` is specific to this format. Splitting for hypothetical reuse is how you get a codebase of tiny functions nobody can follow. Split where the responsibilities differ, not where reuse might one day happen.

One detail in the rewrite worth defending: the `if len(parts) == 2` filter moved into `parse`, so "malformed lines are skipped" is stated in exactly one place instead of being a `continue` buried in a loop that also does arithmetic. Whether skipping silently is the right policy is a separate question — and now it is a question you can see and answer, which is most of the value.

The reasoning

The current code enumerates its exit paths and handles each one. That works exactly as long as the enumeration stays complete, and it will not: the next person adds an early `return` for an empty batch, or a second validation that raises, and the sink leaks. There is nothing in the code that would catch it, because the omission looks like nothing.

`with` inverts the responsibility. `__exit__` runs on every path — normal completion, any exception, `return`, `break`, `continue` — so the guarantee is a property of the resource rather than a discipline the caller has to maintain. That is the same argument as `try`/`finally`, with the advantage that the acquisition and the release are written together and cannot drift apart.

`contextlib.contextmanager` is the cheapest way to add it to a class you own or a third-party object that lacks one. The essential detail is the `try`/`finally` around the `yield`: without the `finally`, an exception in the body skips the cleanup entirely, which is precisely the failure the construct exists to prevent.

Two extensions worth knowing. `contextlib.ExitStack` handles a number of resources decided at run time — a manifest of files, a set of connections — which a fixed `with` statement cannot. And the same pattern generalises past files: transactions that must roll back, locks that must release, temporary directories that must be removed, and partial outputs that must be deleted when a job fails.

The reasoning

The function knows something the caller needs and throws it away. It saw a row with no amount, made a policy decision to skip it, and returned a list that records only the outcome of that decision. Every caller is therefore forced to trust that skipping was fine, because they have no way to check.

A result object carries both outcomes. `LoadResult(written=[...], skipped=[...])` lets one caller ignore the skips, another fail the run when `skipped` is non-empty, and a third write them to a dead-letter table — without the loader having to guess which of those is wanted. The `ok` property gives the common case a name so callers do not each re-derive it.

This is the same principle as the exhausted-retries question and the bare-except question, applied to a signature rather than to control flow: partial success is a real state, and a return type that cannot express it forces the caller to treat it as total success. The published output is deliberately unchanged here — the *caller* still prints the same line — because the point is that the capability is now available, not that the behaviour differs.

The alternative designs are worth naming. Raising on any skip is right when partial results are useless downstream. Returning a `(written, skipped)` tuple works and is worse at three call sites, because nobody remembers the order. And a generator that yields both kinds of outcome is the streaming version, which matters when the batch does not fit in memory.

The reasoning

The polymorphism is in the wrong place. Being liberal about what you accept is genuinely helpful — `str | list[str]` saves callers from wrapping a single value. Being unpredictable about what you return is the opposite: every caller has to reconstruct which branch it took, and the `isinstance` check in the original is the tax being paid at each one.

It is also hard to type honestly. `def normalise(value: str | list[str]) -> str | list[str]` is accurate and useless — it does not say that the output type mirrors the input, so a checker cannot help. Expressing the real relationship needs `@overload` declarations, which is a lot of machinery for a convenience nobody asked for.

The rewrite normalises the input on the first line and returns one type. The caller loses two lines and gains the ability to write `for region in normalise(raw)` without thinking. The published output is identical because the original caller was already converting the string case to a list — which is the evidence that the branch was pure overhead.

The same shape appears elsewhere and is worth recognising: functions that return `None` or a value, a scalar or a dataframe, one row or a list of rows. In every case the fix is the same — pick the general type and always return it, since an empty list or a one-element list is a perfectly good answer and needs no special case.

The reasoning

**The test.** Does anything live between calls? A client, a connection pool, a compiled regex, a cache — those are state, and a class holds them honestly. Three arguments stored and used once is not state; it is arguments, spelled twice.

**Why it matters beyond taste.** A function has one call site and one thing to mock. The class has a constructor and a method, so a test has to build it before it can call it, and every caller now depends on the construction order as well as on the behaviour.

**Where the class is right.** Expensive setup shared across calls, several methods over the same state, or an interface a caller injects — a sink, a clock, an HTTP client. In pipeline code those are exactly the boundaries you want to be able to swap in a test, and a `Protocol`-typed parameter makes them swappable without inheritance.

**And the counter-move.** If the class exists because a function grew five parameters, a class is not the answer either — a config dataclass passed to the function is. Bundling arguments and hiding a call are different problems and only one of them needs a class.

The reasoning

**What the ladder costs.** Every new sink means editing a function that has nothing to do with the new sink. Worse, a test double with exactly the right methods is refused, so tests end up importing production classes just to satisfy an `isinstance` check.

**Protocol, in one sentence.** It is structural typing: anything with the right methods satisfies it, with no inheritance and no import. `class Sink(Protocol): def write(self, rows) -> int: ...` and then `def flush(sink: Sink, rows)`. A type checker verifies the callers; at runtime nothing does.

**When an ABC is the better answer.** When you own every implementation and want a runtime guarantee — an ABC refuses to instantiate a subclass that has not implemented the abstract methods — or when there is shared default behaviour to inherit. A Protocol carries no implementation at all.

**And be honest about the runtime.** A bare Protocol is not `isinstance`-able; `@runtime_checkable` adds that and only checks the methods exist, not their signatures. If you genuinely need a runtime gate, an ABC is the tool. If you need the code to be testable and open to new implementations, the Protocol is.