A small patch can fix the wrong place

A patch can be easy to read and still fix the wrong place.

Suppose a bug report says a page mangles a heading containing an ampersand and angle brackets. The patch escapes the heading text in the page function. The reported page works. Its tests pass. The change barely takes up any space on screen.

There is also a digest that uses the same renderer. Nobody mentioned it in the report.

I no longer use a fixed line limit as a review rule. I still want changes I can understand without unnecessary machinery, but a small diff doesn't tell me whether the author found every affected path. This example is a way to check that claim rather than argue about an ideal file size.

The patch that looks finished

This is a deliberately broken teaching example. Both callers return an HTML fragment; neither opens a browser, writes a file, or sends an email. The contract is that label contains raw plain text.

def heading(label):
    return f"<h1>{label}</h1>"


def page(label):
    return heading(label)


def digest(label):
    return heading(label)

For the label R&D <draft>, both callers return:

<h1>R&D <draft></h1>

The angle brackets belong to the label, but the function puts them into the fragment as markup. We want this instead:

<h1>R&amp;D &lt;draft&gt;</h1>

The tempting patch adds a standard-library import and changes the page caller:

from html import escape


def page(label):
    return heading(escape(label))

That patch works for the page. It leaves digest() unchanged, still passing raw text into the renderer. A page-only test suite gives the patch a clean result without saying anything about the digest.

Follow the call before accepting the fix. Find the definition of heading, then find its references. Here there are only two callers. In a larger repository, use your editor's Find References and check direct references with text search as well. Neither automatically proves the absence of dynamic callers, so inspect dispatch code if the application uses it.

Both paths have the same requirement: treat the label as text inside an HTML heading. That requirement belongs in the renderer that produces the HTML.

Move the fix, then run the same tests

Use the existing heading function. There is no need for a rendering service, a base class, or a new dependency.

from html import escape


def heading(label):
    return f"<h1>{escape(label)}</h1>"


def page(label):
    return heading(label)


def digest(label):
    return heading(label)

Notice that the caller-level escaping is gone. Keeping it would escape the page's text twice. Moving behavior into shared code includes removing obsolete work from its callers.

The companion example contains all three versions and one test file. You need Python 3; everything else comes from its standard library. From the collection's repository root, run each command separately:

cd collection/examples/decision-review
python3 test_review.py before
python3 test_review.py local_patch
python3 test_review.py shared_fix

The first two commands intentionally exit with a failure. These are the results from running the example, not predicted output:

The failing test after the caller-only patch is test_digest_escapes_label. The suite also checks that both callers preserve an ordinary label, Weekly report. Checking unchanged behavior matters when widening a fix: otherwise a repair can merely exchange one broken input for another.

One of the assertions is small enough to read without knowing the test framework:

self.assertEqual(
    app.digest("R&D <draft>"),
    "<h1>R&amp;D &lt;draft&gt;</h1>",
)

The assertion compares the output with the required result. It doesn't demand a particular helper call, so the same test can reject the local patch and accept the shared one.

What I'd ask in the review

My first comment would be specific: "The digest also calls heading. Can we put escaping there and run the label cases through both callers?"

Then I'd check the input contract. The shared fix is correct here because both callers supply raw text. If another caller supplies HTML, that is a different contract to resolve before moving the behavior. Don't fix double escaping by blindly unescaping input; establish which layer owns conversion to HTML.

For your next patch, write down the behavior that must hold, identify the function that owns it, and list its callers. Keep a test that fails on the reported path. Add the sibling path and a normal input, then run those tests against the proposed change. If the fix changes a shared contract, inspect all its callers before treating a passing test as sufficient evidence.

This demonstration only tests string output for HTML text content. It does not test a browser or establish a general HTML security boundary. URLs, scripts, and intentionally supplied markup need their own rules. In an application with an autoescaping template system, use that system's escaping contract rather than adding this helper beside it.

The useful review finding here is quite modest: the page and digest needed the same correction, and the renderer already gave us a place to make it once.